Patchwork [7/7] smbios: Check R in -smbios type=0, release=R parses okay

login
register
mail settings
Submitter Markus Armbruster
Date June 6, 2013, 4:27 p.m.
Message ID <1370536046-15125-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/249476/
State New
Headers show

Comments

Markus Armbruster - June 6, 2013, 4:27 p.m.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/smbios.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Laszlo Ersek - June 6, 2013, 6:39 p.m.
On 06/06/13 18:27, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/smbios.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 68bd6d0..88a1360 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
>                                       bios_release_date_str),
>                           buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "release", t)) {
> -        sscanf(buf, "%hhd.%hhd", &major, &minor);
> +        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
> +            error_report("Invalid release");
> +            exit(1);
> +        }
>          smbios_add_field(0, offsetof(struct smbios_type_0,
>                                       system_bios_major_release),
>                           &major, 1);
> 

Right. OTOH if any of the decimal strings provided doesn't fit into the
space provided (eg. you pass "256" for an "unsigned char" which happens
to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
used with "untrusted" data. ("... if the result of the conversion cannot
be represented in the space provided, the behavior is undefined.")

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Markus Armbruster - June 6, 2013, 8:02 p.m.
Laszlo Ersek <lersek@redhat.com> writes:

> On 06/06/13 18:27, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/i386/smbios.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 68bd6d0..88a1360 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
>>                                       bios_release_date_str),
>>                           buf, strlen(buf) + 1);
>>      if (get_param_value(buf, sizeof(buf), "release", t)) {
>> -        sscanf(buf, "%hhd.%hhd", &major, &minor);
>> +        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
>> +            error_report("Invalid release");
>> +            exit(1);
>> +        }
>>          smbios_add_field(0, offsetof(struct smbios_type_0,
>>                                       system_bios_major_release),
>>                           &major, 1);
>> 
>
> Right. OTOH if any of the decimal strings provided doesn't fit into the
> space provided (eg. you pass "256" for an "unsigned char" which happens
> to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
> used with "untrusted" data. ("... if the result of the conversion cannot
> be represented in the space provided, the behavior is undefined.")

Yes, this isn't rigorous parsing.  It improves the code from "brazenly
careless" to the more common (in QEMU) "quick but sloppy".

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

Thanks for the review!

Patch

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 68bd6d0..88a1360 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -140,7 +140,10 @@  static void smbios_build_type_0_fields(const char *t)
                                      bios_release_date_str),
                          buf, strlen(buf) + 1);
     if (get_param_value(buf, sizeof(buf), "release", t)) {
-        sscanf(buf, "%hhd.%hhd", &major, &minor);
+        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
+            error_report("Invalid release");
+            exit(1);
+        }
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
                          &major, 1);