| Message ID | 1370536046-15125-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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!
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);
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/i386/smbios.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)