Patchwork acpi: fix file size check with -acpitable.

login
register
mail settings
Submitter Isaku Yamahata
Date July 29, 2010, 9:08 a.m.
Message ID <20100729090842.GL31169@valinux.co.jp>
Download mbox | patch
Permalink /patch/60199/
State New
Headers show

Comments

Isaku Yamahata - July 29, 2010, 9:08 a.m.
acpi table file can be modified during load so file size check
should be more strict.
pointer calculation should be after qemu_realloc(). not before realloc().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/acpi.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)
Isaku Yamahata - Aug. 24, 2010, 5:06 a.m.
ping.

On Thu, Jul 29, 2010 at 06:08:42PM +0900, Isaku Yamahata wrote:
> acpi table file can be modified during load so file size check
> should be more strict.
> pointer calculation should be after qemu_realloc(). not before realloc().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/acpi.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index c7044b1..069e05f 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -50,6 +50,8 @@ int acpi_table_add(const char *t)
>      char buf[1024], *p, *f;
>      struct acpi_table_header acpi_hdr;
>      unsigned long val;
> +    uint32_t length;
> +    struct acpi_table_header *acpi_hdr_p;
>      size_t off;
>  
>      memset(&acpi_hdr, 0, sizeof(acpi_hdr));
> @@ -108,7 +110,7 @@ int acpi_table_add(const char *t)
>           buf[0] = '\0';
>      }
>  
> -    acpi_hdr.length = sizeof(acpi_hdr);
> +    length = sizeof(acpi_hdr);
>  
>      f = buf;
>      while (buf[0]) {
> @@ -120,7 +122,7 @@ int acpi_table_add(const char *t)
>              fprintf(stderr, "Can't stat file '%s': %s\n", f, strerror(errno));
>              goto out;
>          }
> -        acpi_hdr.length += s.st_size;
> +        length += s.st_size;
>          if (!n)
>              break;
>          *n = ':';
> @@ -131,12 +133,12 @@ int acpi_table_add(const char *t)
>          acpi_tables_len = sizeof(uint16_t);
>          acpi_tables = qemu_mallocz(acpi_tables_len);
>      }
> +    acpi_tables = qemu_realloc(acpi_tables,
> +                               acpi_tables_len + sizeof(uint16_t) + length);
>      p = acpi_tables + acpi_tables_len;
> -    acpi_tables_len += sizeof(uint16_t) + acpi_hdr.length;
> -    acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len);
> +    acpi_tables_len += sizeof(uint16_t) + length;
>  
> -    acpi_hdr.length = cpu_to_le32(acpi_hdr.length);
> -    *(uint16_t*)p = acpi_hdr.length;
> +    *(uint16_t*)p = cpu_to_le32(length);
>      p += sizeof(uint16_t);
>      memcpy(p, &acpi_hdr, sizeof(acpi_hdr));
>      off = sizeof(acpi_hdr);
> @@ -157,7 +159,9 @@ int acpi_table_add(const char *t)
>              goto out;
>          }
>  
> -        do {
> +        /* off < length is necessary because file size can be changed
> +           under our foot */
> +        while(s.st_size && off < length); {
>              int r;
>              r = read(fd, p + off, s.st_size);
>              if (r > 0) {
> @@ -167,15 +171,21 @@ int acpi_table_add(const char *t)
>                  close(fd);
>                  goto out;
>              }
> -        } while(s.st_size);
> +        }
>  
>          close(fd);
>          if (!n)
>              break;
>          f = n + 1;
>      }
> +    if (off < length) {
> +        /* don't pass random value in process to guest */
> +        memset(p + off, 0, length - off);
> +    }
>  
> -    ((struct acpi_table_header*)p)->checksum = acpi_checksum((uint8_t*)p, off);
> +    acpi_hdr_p = (struct acpi_table_header*)p;
> +    acpi_hdr_p->length = cpu_to_le32(length);
> +    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
>      /* increase number of tables */
>      (*(uint16_t*)acpi_tables) =
>  	    cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1);
> -- 
> 1.7.1.1
>
Blue Swirl - Aug. 31, 2010, 6:27 p.m.
Thanks, applied.

On Tue, Aug 24, 2010 at 5:06 AM, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> ping.
>
> On Thu, Jul 29, 2010 at 06:08:42PM +0900, Isaku Yamahata wrote:
>> acpi table file can be modified during load so file size check
>> should be more strict.
>> pointer calculation should be after qemu_realloc(). not before realloc().
>>
>> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> ---
>>  hw/acpi.c |   28 +++++++++++++++++++---------
>>  1 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/acpi.c b/hw/acpi.c
>> index c7044b1..069e05f 100644
>> --- a/hw/acpi.c
>> +++ b/hw/acpi.c
>> @@ -50,6 +50,8 @@ int acpi_table_add(const char *t)
>>      char buf[1024], *p, *f;
>>      struct acpi_table_header acpi_hdr;
>>      unsigned long val;
>> +    uint32_t length;
>> +    struct acpi_table_header *acpi_hdr_p;
>>      size_t off;
>>
>>      memset(&acpi_hdr, 0, sizeof(acpi_hdr));
>> @@ -108,7 +110,7 @@ int acpi_table_add(const char *t)
>>           buf[0] = '\0';
>>      }
>>
>> -    acpi_hdr.length = sizeof(acpi_hdr);
>> +    length = sizeof(acpi_hdr);
>>
>>      f = buf;
>>      while (buf[0]) {
>> @@ -120,7 +122,7 @@ int acpi_table_add(const char *t)
>>              fprintf(stderr, "Can't stat file '%s': %s\n", f, strerror(errno));
>>              goto out;
>>          }
>> -        acpi_hdr.length += s.st_size;
>> +        length += s.st_size;
>>          if (!n)
>>              break;
>>          *n = ':';
>> @@ -131,12 +133,12 @@ int acpi_table_add(const char *t)
>>          acpi_tables_len = sizeof(uint16_t);
>>          acpi_tables = qemu_mallocz(acpi_tables_len);
>>      }
>> +    acpi_tables = qemu_realloc(acpi_tables,
>> +                               acpi_tables_len + sizeof(uint16_t) + length);
>>      p = acpi_tables + acpi_tables_len;
>> -    acpi_tables_len += sizeof(uint16_t) + acpi_hdr.length;
>> -    acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len);
>> +    acpi_tables_len += sizeof(uint16_t) + length;
>>
>> -    acpi_hdr.length = cpu_to_le32(acpi_hdr.length);
>> -    *(uint16_t*)p = acpi_hdr.length;
>> +    *(uint16_t*)p = cpu_to_le32(length);
>>      p += sizeof(uint16_t);
>>      memcpy(p, &acpi_hdr, sizeof(acpi_hdr));
>>      off = sizeof(acpi_hdr);
>> @@ -157,7 +159,9 @@ int acpi_table_add(const char *t)
>>              goto out;
>>          }
>>
>> -        do {
>> +        /* off < length is necessary because file size can be changed
>> +           under our foot */
>> +        while(s.st_size && off < length); {
>>              int r;
>>              r = read(fd, p + off, s.st_size);
>>              if (r > 0) {
>> @@ -167,15 +171,21 @@ int acpi_table_add(const char *t)
>>                  close(fd);
>>                  goto out;
>>              }
>> -        } while(s.st_size);
>> +        }
>>
>>          close(fd);
>>          if (!n)
>>              break;
>>          f = n + 1;
>>      }
>> +    if (off < length) {
>> +        /* don't pass random value in process to guest */
>> +        memset(p + off, 0, length - off);
>> +    }
>>
>> -    ((struct acpi_table_header*)p)->checksum = acpi_checksum((uint8_t*)p, off);
>> +    acpi_hdr_p = (struct acpi_table_header*)p;
>> +    acpi_hdr_p->length = cpu_to_le32(length);
>> +    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
>>      /* increase number of tables */
>>      (*(uint16_t*)acpi_tables) =
>>           cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1);
>> --
>> 1.7.1.1
>>
>
> --
> yamahata
>
>

Patch

diff --git a/hw/acpi.c b/hw/acpi.c
index c7044b1..069e05f 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -50,6 +50,8 @@  int acpi_table_add(const char *t)
     char buf[1024], *p, *f;
     struct acpi_table_header acpi_hdr;
     unsigned long val;
+    uint32_t length;
+    struct acpi_table_header *acpi_hdr_p;
     size_t off;
 
     memset(&acpi_hdr, 0, sizeof(acpi_hdr));
@@ -108,7 +110,7 @@  int acpi_table_add(const char *t)
          buf[0] = '\0';
     }
 
-    acpi_hdr.length = sizeof(acpi_hdr);
+    length = sizeof(acpi_hdr);
 
     f = buf;
     while (buf[0]) {
@@ -120,7 +122,7 @@  int acpi_table_add(const char *t)
             fprintf(stderr, "Can't stat file '%s': %s\n", f, strerror(errno));
             goto out;
         }
-        acpi_hdr.length += s.st_size;
+        length += s.st_size;
         if (!n)
             break;
         *n = ':';
@@ -131,12 +133,12 @@  int acpi_table_add(const char *t)
         acpi_tables_len = sizeof(uint16_t);
         acpi_tables = qemu_mallocz(acpi_tables_len);
     }
+    acpi_tables = qemu_realloc(acpi_tables,
+                               acpi_tables_len + sizeof(uint16_t) + length);
     p = acpi_tables + acpi_tables_len;
-    acpi_tables_len += sizeof(uint16_t) + acpi_hdr.length;
-    acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len);
+    acpi_tables_len += sizeof(uint16_t) + length;
 
-    acpi_hdr.length = cpu_to_le32(acpi_hdr.length);
-    *(uint16_t*)p = acpi_hdr.length;
+    *(uint16_t*)p = cpu_to_le32(length);
     p += sizeof(uint16_t);
     memcpy(p, &acpi_hdr, sizeof(acpi_hdr));
     off = sizeof(acpi_hdr);
@@ -157,7 +159,9 @@  int acpi_table_add(const char *t)
             goto out;
         }
 
-        do {
+        /* off < length is necessary because file size can be changed
+           under our foot */
+        while(s.st_size && off < length); {
             int r;
             r = read(fd, p + off, s.st_size);
             if (r > 0) {
@@ -167,15 +171,21 @@  int acpi_table_add(const char *t)
                 close(fd);
                 goto out;
             }
-        } while(s.st_size);
+        }
 
         close(fd);
         if (!n)
             break;
         f = n + 1;
     }
+    if (off < length) {
+        /* don't pass random value in process to guest */
+        memset(p + off, 0, length - off);
+    }
 
-    ((struct acpi_table_header*)p)->checksum = acpi_checksum((uint8_t*)p, off);
+    acpi_hdr_p = (struct acpi_table_header*)p;
+    acpi_hdr_p->length = cpu_to_le32(length);
+    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
     /* increase number of tables */
     (*(uint16_t*)acpi_tables) =
 	    cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1);