diff mbox

[1/2] fw_cfg: Splash image loader can overrun a stack variable, fix

Message ID 1358961909-17519-2-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Jan. 23, 2013, 5:25 p.m. UTC 
read_splashfile() passes the address of an int variable as size_t *
parameter to g_file_get_contents(), with a cast to gag the compiler.

No problem on machines where sizeof(size_t) == sizeof(int).

Happens to work on my x86_64 box (64 bit little endian): the least
significant 32 bits of the file size end up in the right place
(caller's variable file_size), and the most significant 32 bits
clobber a place that gets assigned to before its next use (caller's
variable file_type).

I'd expect it to break on a 64 bit big-endian box.

Fix up the variable types and drop the problematic cast.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c             | 7 ++++---
 include/sysemu/sysemu.h | 2 +-
 vl.c                    | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

Comments

Laszlo Ersek Jan. 24, 2013, 11:08 a.m. UTC | #1 
On 01/23/13 18:25, Markus Armbruster wrote:
> read_splashfile() passes the address of an int variable as size_t *
> parameter to g_file_get_contents(), with a cast to gag the compiler.
> 
> No problem on machines where sizeof(size_t) == sizeof(int).
> 
> Happens to work on my x86_64 box (64 bit little endian): the least
> significant 32 bits of the file size end up in the right place
> (caller's variable file_size), and the most significant 32 bits
> clobber a place that gets assigned to before its next use (caller's
> variable file_type).
> 
> I'd expect it to break on a 64 bit big-endian box.
> 
> Fix up the variable types and drop the problematic cast.
> 

Ultimately fw_cfg_add_file() and fw_cfg_add_bytes() (reasonably) convert
(truncate) the size to uint32_t, but that's completely orthogonal.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
diff mbox

Patch

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index e4dc7c3..b7da5c7 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -54,7 +54,8 @@  struct FWCfgState {
 #define JPG_FILE 0
 #define BMP_FILE 1
 
-static char *read_splashfile(char *filename, int *file_sizep, int *file_typep)
+static char *read_splashfile(char *filename, size_t *file_sizep,
+                             int *file_typep)
 {
     GError *err = NULL;
     gboolean res;
@@ -63,7 +64,7 @@  static char *read_splashfile(char *filename, int *file_sizep, int *file_typep)
     unsigned int filehead = 0;
     int bmp_bpp;
 
-    res = g_file_get_contents(filename, &content, (gsize *)file_sizep, &err);
+    res = g_file_get_contents(filename, &content, file_sizep, &err);
     if (res == FALSE) {
         error_report("failed to read splash file '%s'", filename);
         g_error_free(err);
@@ -111,7 +112,7 @@  static void fw_cfg_bootsplash(FWCfgState *s)
     const char *boot_splash_filename = NULL;
     char *p;
     char *filename, *file_data;
-    int file_size;
+    size_t file_size;
     int file_type = -1;
     const char *temp;
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 337ce7d..1d9599e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -122,7 +122,7 @@  extern int semihosting_enabled;
 extern int old_param;
 extern int boot_menu;
 extern uint8_t *boot_splash_filedata;
-extern int boot_splash_filedata_size;
+extern size_t boot_splash_filedata_size;
 extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClock *rtc_clock;
 
diff --git a/vl.c b/vl.c
index 4ee1302..7aab73b 100644
--- a/vl.c
+++ b/vl.c
@@ -231,7 +231,7 @@  unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
 uint8_t *boot_splash_filedata;
-int boot_splash_filedata_size;
+size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
 
 typedef struct FWBootEntry FWBootEntry;