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

Submitted by Markus Armbruster on June 6, 2013, 4:27 p.m.

Details

Message ID 1370536046-15125-8-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);