[Qemu-devel] [PATCH] Remove most uses of phys_ram_base in hw/pc.c

[Ian Jackson]

I wrote:
> So I think it would be nice to have that change in qemu upstream.
> While it doesn't directly enable anything useful right away, the
> result is slightly cleaner.  I'll submit a proper patch shortly.

In the attached patch, I remove all the direct uses of phys_ram_base
from hw/pc.c, except for those presently needed to construct the
arguments to the vga init functions.

This involved:
 * Getting rid of various additions and subtractions of phys_ram_base
 * Changing the types of the guest physical addresses in load_linux
   from uint8_t* to target_phys_addr_t
 * Replacing calls to memcpy and pstrcpy with
   cpu_physical_memory_write (and a new pstrcpy_targphys function)
 * Replacing most calls to fread with a new fread_targphys function
 * Deprecating load_image in favour of a new load_image_targphys
 * Removing (rather than fixing up) the unused function load_kernel

I noticed that load_image doesn't take a buffer size argument - it
just overwrites the destination buffer with file data, extending as
long as the file happens to be.  In most cases this is probably not an
exploitable vulnerability, but it seems poor practice.  Hence
load_image_targphys's extra argument.

I hope this meets with your approval.

Thanks,
Ian.

Signed-off-by: Ian Jackson <address@hidden>
---
 hw/pc.c  |   98 ++++++++++++++++++++++++++-----------------------------------
 loader.c |   43 +++++++++++++++++++++++++++
 sysemu.h |    7 ++++-
 3 files changed, 91 insertions(+), 57 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 5333f47..6a2d9ea 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -435,36 +435,6 @@ static void generate_bootsect(uint32_t gpr[8], uint16_t 
segs[6], uint16_t ip)
     bdrv_set_boot_sector(drives_table[hda].bdrv, bootsect, sizeof(bootsect));
 }
 
-static int load_kernel(const char *filename, uint8_t *addr,
-                       uint8_t *real_addr)
-{
-    int fd, size;
-    int setup_sects;
-
-    fd = open(filename, O_RDONLY | O_BINARY);
-    if (fd < 0)
-        return -1;
-
-    /* load 16 bit code */
-    if (qemu_read_ok(fd, real_addr, 512) < 0)
-        goto fail;
-    setup_sects = real_addr[0x1F1];
-    if (!setup_sects)
-        setup_sects = 4;
-    if (qemu_read_ok(fd, real_addr + 512, setup_sects * 512) < 0)
-        goto fail;
-
-    /* load 32 bit code */
-    size = qemu_read(fd, addr, 16 * 1024 * 1024);
-    if (size < 0)
-        goto fail;
-    close(fd);
-    return size;
- fail:
-    close(fd);
-    return -1;
-}
-
 static long get_file_size(FILE *f)
 {
     long where, size;
@@ -479,6 +449,22 @@ static long get_file_size(FILE *f)
     return size;
 }
 
+static void pstrcpy_targphys(target_phys_addr_t dest, int buf_size,
+                            const char *source)
+{
+    static const char nul_byte;
+    const char *nulp;
+
+    if (buf_size <= 0) return;
+    nulp = memchr(source, 0, buf_size);
+    if (nulp) {
+       cpu_physical_memory_write(dest, source, (nulp - source) + 1);
+    } else {
+       cpu_physical_memory_write(dest, source, buf_size - 1);
+       cpu_physical_memory_write(dest, &nul_byte, 1);
+    }
+}
+
 static void load_linux(const char *kernel_filename,
                       const char *initrd_filename,
                       const char *kernel_cmdline)
@@ -490,7 +476,7 @@ static void load_linux(const char *kernel_filename,
     int setup_size, kernel_size, initrd_size, cmdline_size;
     uint32_t initrd_max;
     uint8_t header[1024];
-    uint8_t *real_addr, *prot_addr, *cmdline_addr, *initrd_addr;
+    target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr;
     FILE *f, *fi;
 
     /* Align to 16 bytes as a paranoia measure */
@@ -516,19 +502,19 @@ static void load_linux(const char *kernel_filename,
 
     if (protocol < 0x200 || !(header[0x211] & 0x01)) {
        /* Low kernel */
-       real_addr    = phys_ram_base + 0x90000;
-       cmdline_addr = phys_ram_base + 0x9a000 - cmdline_size;
-       prot_addr    = phys_ram_base + 0x10000;
+       real_addr    = 0x90000;
+       cmdline_addr = 0x9a000 - cmdline_size;
+       prot_addr    = 0x10000;
     } else if (protocol < 0x202) {
        /* High but ancient kernel */
-       real_addr    = phys_ram_base + 0x90000;
-       cmdline_addr = phys_ram_base + 0x9a000 - cmdline_size;
-       prot_addr    = phys_ram_base + 0x100000;
+       real_addr    = 0x90000;
+       cmdline_addr = 0x9a000 - cmdline_size;
+       prot_addr    = 0x100000;
     } else {
        /* High and recent kernel */
-       real_addr    = phys_ram_base + 0x10000;
-       cmdline_addr = phys_ram_base + 0x20000;
-       prot_addr    = phys_ram_base + 0x100000;
+       real_addr    = 0x10000;
+       cmdline_addr = 0x20000;
+       prot_addr    = 0x100000;
     }
 
 #if 0
@@ -536,9 +522,9 @@ static void load_linux(const char *kernel_filename,
            "qemu: real_addr     = %#zx\n"
            "qemu: cmdline_addr  = %#zx\n"
            "qemu: prot_addr     = %#zx\n",
-           real_addr-phys_ram_base,
-           cmdline_addr-phys_ram_base,
-           prot_addr-phys_ram_base);
+           real_addr,
+           cmdline_addr,
+           prot_addr);
 #endif
 
     /* highest address for loading the initrd */
@@ -551,10 +537,10 @@ static void load_linux(const char *kernel_filename,
        initrd_max = ram_size-ACPI_DATA_SIZE-1;
 
     /* kernel command line */
-    pstrcpy((char*)cmdline_addr, 4096, kernel_cmdline);
+    pstrcpy_targphys(cmdline_addr, 4096, kernel_cmdline);
 
     if (protocol >= 0x202) {
-       stl_p(header+0x228, cmdline_addr-phys_ram_base);
+       stl_p(header+0x228, cmdline_addr);
     } else {
        stw_p(header+0x20, 0xA33F);
        stw_p(header+0x22, cmdline_addr-real_addr);
@@ -588,24 +574,24 @@ static void load_linux(const char *kernel_filename,
        }
 
        initrd_size = get_file_size(fi);
-       initrd_addr = phys_ram_base + ((initrd_max-initrd_size) & ~4095);
+       initrd_addr = ((initrd_max-initrd_size) & ~4095);
 
        fprintf(stderr, "qemu: loading initrd (%#x bytes) at %#zx\n",
-               initrd_size, initrd_addr-phys_ram_base);
+               initrd_size, initrd_addr);
 
-       if (fread(initrd_addr, 1, initrd_size, fi) != initrd_size) {
+       if (!fread_targphys_ok(initrd_addr, initrd_size, fi)) {
            fprintf(stderr, "qemu: read error on initial ram disk '%s'\n",
                    initrd_filename);
            exit(1);
        }
        fclose(fi);
 
-       stl_p(header+0x218, initrd_addr-phys_ram_base);
+       stl_p(header+0x218, initrd_addr);
        stl_p(header+0x21c, initrd_size);
     }
 
     /* store the finalized header and load the rest of the kernel */
-    memcpy(real_addr, header, 1024);
+    cpu_physical_memory_write(real_addr, header, 1024);
 
     setup_size = header[0x1f1];
     if (setup_size == 0)
@@ -614,8 +600,8 @@ static void load_linux(const char *kernel_filename,
     setup_size = (setup_size+1)*512;
     kernel_size -= setup_size; /* Size of protected-mode code */
 
-    if (fread(real_addr+1024, 1, setup_size-1024, f) != setup_size-1024 ||
-       fread(prot_addr, 1, kernel_size, f) != kernel_size) {
+    if (!fread_targphys_ok(real_addr+1024, setup_size-1024, f) ||
+       !fread_targphys_ok(prot_addr, kernel_size, f)) {
        fprintf(stderr, "qemu: read error on kernel '%s'\n",
                kernel_filename);
        exit(1);
@@ -623,7 +609,7 @@ static void load_linux(const char *kernel_filename,
     fclose(f);
 
     /* generate bootsector to set up the initial register state */
-    real_seg = (real_addr-phys_ram_base) >> 4;
+    real_seg = real_addr >> 4;
     seg[0] = seg[2] = seg[3] = seg[4] = seg[4] = real_seg;
     seg[1] = real_seg+0x20;    /* CS */
     memset(gpr, 0, sizeof gpr);
@@ -764,7 +750,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
         goto bios_error;
     }
     bios_offset = qemu_ram_alloc(bios_size);
-    ret = load_image(buf, phys_ram_base + bios_offset);
+    ret = load_image_targphys(buf, bios_offset, bios_size);
     if (ret != bios_size) {
     bios_error:
         fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", buf);
@@ -782,7 +768,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
         goto vga_bios_error;
     vga_bios_offset = qemu_ram_alloc(65536);
 
-    ret = load_image(buf, phys_ram_base + vga_bios_offset);
+    ret = load_image_targphys(buf, vga_bios_offset, vga_bios_size);
     if (ret != vga_bios_size) {
     vga_bios_error:
         fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
@@ -818,7 +804,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
             if (size > (0x10000 - offset))
                 goto option_rom_error;
             option_rom_offset = qemu_ram_alloc(size);
-            ret = load_image(option_rom[i], phys_ram_base + option_rom_offset);
+            ret = load_image_targphys(option_rom[i], option_rom_offset, size);
             if (ret != size) {
             option_rom_error:
                 fprintf(stderr, "Too many option ROMS\n");
diff --git a/loader.c b/loader.c
index 813756e..5462d24 100644
--- a/loader.c
+++ b/loader.c
@@ -39,6 +39,7 @@ int get_image_size(const char *filename)
 }
 
 /* return the size or -1 if error */
+/* deprecated, because caller does not specify buffer size! */
 int load_image(const char *filename, uint8_t *addr)
 {
     int fd, size;
@@ -55,6 +56,48 @@ int load_image(const char *filename, uint8_t *addr)
     return size;
 }
 
+/* return the amount read, just like fread.  0 may mean error or eof */
+int fread_targphys(target_phys_addr_t dst_addr, size_t nbytes, FILE *f)
+{
+    unsigned char buf[4096];
+    target_phys_addr_t dst_begin = dst_addr;
+    size_t want, did;
+
+    while (nbytes) {
+       want = nbytes > sizeof(buf) ? sizeof(buf) : nbytes;
+       did = fread(buf, 1, want, f);
+       if (did != want) break;
+       
+       cpu_physical_memory_write(dst_addr, buf, did);
+       dst_addr += did;
+       nbytes -= did;
+    }
+    return dst_addr - dst_begin;
+}
+
+/* returns 0 on error, 1 if ok */
+int fread_targphys_ok(target_phys_addr_t dst_addr, size_t nbytes, FILE *f)
+{
+    return fread_targphys(dst_addr, nbytes, f) == nbytes;
+}
+    
+/* return the size or -1 if error */
+int load_image_targphys(const char *filename,
+                       target_phys_addr_t addr, int max_sz)
+{
+    FILE *f;
+    size_t got;
+
+    f = fopen(filename, "rb");
+    if (!f) return -1;
+
+    got = fread_targphys(addr, max_sz, f);
+    if (ferror(f)) { fclose(f); return -1; }
+    fclose(f);
+
+    return got;
+}
+
 /* A.OUT loader */
 
 struct exec
diff --git a/sysemu.h b/sysemu.h
index 296f179..e62f832 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -150,11 +150,16 @@ extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 #ifdef NEED_CPU_H
 /* loader.c */
 int get_image_size(const char *filename);
-int load_image(const char *filename, uint8_t *addr);
+int load_image(const char *filename, uint8_t *addr); /* deprecated */
+int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
 int load_elf(const char *filename, int64_t virt_to_phys_addend,
              uint64_t *pentry, uint64_t *lowaddr, uint64_t *highaddr);
 int load_aout(const char *filename, uint8_t *addr);
 int load_uboot(const char *filename, target_ulong *ep, int *is_linux);
+
+int fread_targphys(target_phys_addr_t dst_addr, size_t nbytes, FILE *f);
+int fread_targphys_ok(target_phys_addr_t dst_addr, size_t nbytes, FILE *f);
+
 #endif
 
 #ifdef HAS_AUDIO
-- 
1.4.4.4