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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 06/06/13 18:27, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks for the review!