* [PATCH] firmware: dmi: Fortify entry point length checks
@ 2022-09-07 8:30 Jean Delvare
2022-09-07 14:52 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2022-09-07 8:30 UTC (permalink / raw)
To: LKML; +Cc: Linus Torvalds, Andy Shevchenko
Ensure that the SMBIOS entry point is long enough to include all the
fields we need. Otherwise it is pointless to even attempt to verify
its checksum.
Also fix the maximum length check, which is technically 32, not 31.
It does not matter in practice as the only valid values are 31 (for
SMBIOS 2.x) and 24 (for SMBIOS 3.x), but let's still have the check
right in case new fields are added to either structure in the future.
Signed-off-by: Jean Delvare <jdelvare@suse.de>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/lkml/20220823094857.27f3d924@endymion.delvare/T/
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/firmware/dmi_scan.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--- linux-5.19.orig/drivers/firmware/dmi_scan.c 2022-09-06 15:22:44.279728845 +0200
+++ linux-5.19/drivers/firmware/dmi_scan.c 2022-09-07 10:16:55.743362320 +0200
@@ -568,7 +568,8 @@ static int __init dmi_present(const u8 *
u32 smbios_ver;
if (memcmp(buf, "_SM_", 4) == 0 &&
- buf[5] < 32 && dmi_checksum(buf, buf[5])) {
+ buf[5] >= 31 && buf[5] <= 32 &&
+ dmi_checksum(buf, buf[5])) {
smbios_ver = get_unaligned_be16(buf + 6);
smbios_entry_point_size = buf[5];
memcpy(smbios_entry_point, buf, smbios_entry_point_size);
@@ -629,7 +630,8 @@ static int __init dmi_present(const u8 *
static int __init dmi_smbios3_present(const u8 *buf)
{
if (memcmp(buf, "_SM3_", 5) == 0 &&
- buf[6] < 32 && dmi_checksum(buf, buf[6])) {
+ buf[6] >= 24 && buf[6] <= 32 &&
+ dmi_checksum(buf, buf[6])) {
dmi_ver = get_unaligned_be24(buf + 7);
dmi_num = 0; /* No longer specified */
dmi_len = get_unaligned_le32(buf + 12);
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] firmware: dmi: Fortify entry point length checks
2022-09-07 8:30 [PATCH] firmware: dmi: Fortify entry point length checks Jean Delvare
@ 2022-09-07 14:52 ` Andy Shevchenko
2022-09-07 15:21 ` Jean Delvare
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2022-09-07 14:52 UTC (permalink / raw)
To: Jean Delvare; +Cc: LKML, Linus Torvalds
On Wed, Sep 7, 2022 at 11:30 AM Jean Delvare <jdelvare@suse.de> wrote:
>
> Ensure that the SMBIOS entry point is long enough to include all the
> fields we need. Otherwise it is pointless to even attempt to verify
> its checksum.
>
> Also fix the maximum length check, which is technically 32, not 31.
> It does not matter in practice as the only valid values are 31 (for
> SMBIOS 2.x)
"NOTE: This value was incorrectly stated in version 2.1 of this specification as
1Eh. Because of this, there might be version 2.1 implementations that
use either the 1Eh or the 1Fh value, but version 2.2 or later
implementations must use the 1Fh value."
> and 24 (for SMBIOS 3.x), but let's still have the check
> right in case new fields are added to either structure in the future.
Thanks, makes sense to me. But probably needs more work :-)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: dmi: Fortify entry point length checks
2022-09-07 14:52 ` Andy Shevchenko
@ 2022-09-07 15:21 ` Jean Delvare
2022-09-07 15:48 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2022-09-07 15:21 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: LKML, Linus Torvalds
Hi Andy,
On Wed, 7 Sep 2022 17:52:10 +0300, Andy Shevchenko wrote:
> On Wed, Sep 7, 2022 at 11:30 AM Jean Delvare <jdelvare@suse.de> wrote:
> >
> > Ensure that the SMBIOS entry point is long enough to include all the
> > fields we need. Otherwise it is pointless to even attempt to verify
> > its checksum.
> >
> > Also fix the maximum length check, which is technically 32, not 31.
> > It does not matter in practice as the only valid values are 31 (for
> > SMBIOS 2.x)
>
> "NOTE: This value was incorrectly stated in version 2.1 of this specification as
> 1Eh. Because of this, there might be version 2.1 implementations that
> use either the 1Eh or the 1Fh value, but version 2.2 or later
> implementations must use the 1Fh value."
Good point, so maybe we should accept 0x1E and treat is silently as
0x1F (which is what we have been doing implicitly so far) for maximum
compatibility?
> > and 24 (for SMBIOS 3.x), but let's still have the check
> > right in case new fields are added to either structure in the
> > future.
>
> Thanks, makes sense to me. But probably needs more work :-)
Of course more work would presumably be needed there, but I assume such
changes would have to be compatible with previous implementations, so
we don't want to choke on a length check for no reason.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: dmi: Fortify entry point length checks
2022-09-07 15:21 ` Jean Delvare
@ 2022-09-07 15:48 ` Andy Shevchenko
2022-09-07 16:09 ` Jean Delvare
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2022-09-07 15:48 UTC (permalink / raw)
To: Jean Delvare; +Cc: LKML, Linus Torvalds
On Wed, Sep 7, 2022 at 6:21 PM Jean Delvare <jdelvare@suse.de> wrote:
> On Wed, 7 Sep 2022 17:52:10 +0300, Andy Shevchenko wrote:
> > On Wed, Sep 7, 2022 at 11:30 AM Jean Delvare <jdelvare@suse.de> wrote:
...
> > > Also fix the maximum length check, which is technically 32, not 31.
> > > It does not matter in practice as the only valid values are 31 (for
> > > SMBIOS 2.x)
> >
> > "NOTE: This value was incorrectly stated in version 2.1 of this specification as
> > 1Eh. Because of this, there might be version 2.1 implementations that
> > use either the 1Eh or the 1Fh value, but version 2.2 or later
> > implementations must use the 1Fh value."
>
> Good point, so maybe we should accept 0x1E and treat is silently as
> 0x1F (which is what we have been doing implicitly so far) for maximum
> compatibility?
At least the previous comparison covers this case, if I'm not mistaken.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: dmi: Fortify entry point length checks
2022-09-07 15:48 ` Andy Shevchenko
@ 2022-09-07 16:09 ` Jean Delvare
2022-09-07 16:22 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2022-09-07 16:09 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: LKML, Linus Torvalds
On Wed, 7 Sep 2022 18:48:03 +0300, Andy Shevchenko wrote:
> On Wed, Sep 7, 2022 at 6:21 PM Jean Delvare <jdelvare@suse.de> wrote:
> > On Wed, 7 Sep 2022 17:52:10 +0300, Andy Shevchenko wrote:
> > > "NOTE: This value was incorrectly stated in version 2.1 of this specification as
> > > 1Eh. Because of this, there might be version 2.1 implementations that
> > > use either the 1Eh or the 1Fh value, but version 2.2 or later
> > > implementations must use the 1Fh value."
> >
> > Good point, so maybe we should accept 0x1E and treat is silently as
> > 0x1F (which is what we have been doing implicitly so far) for maximum
> > compatibility?
>
> At least the previous comparison covers this case, if I'm not mistaken.
Before my proposed change, yes. After my proposed change, no longer.
Let's not risk a regression, I'll change the check to:
if (memcmp(buf, "_SM_", 4) == 0 &&
buf[5] >= 30 && buf[5] <= 32 &&
dmi_checksum(buf, buf[5])) {
I'll also add a comment stating why we are allowing length 30.
Thanks for the valuable feedback,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] firmware: dmi: Fortify entry point length checks
2022-09-07 16:09 ` Jean Delvare
@ 2022-09-07 16:22 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2022-09-07 16:22 UTC (permalink / raw)
To: Jean Delvare; +Cc: LKML, Linus Torvalds
On Wed, Sep 7, 2022 at 7:09 PM Jean Delvare <jdelvare@suse.de> wrote:
> On Wed, 7 Sep 2022 18:48:03 +0300, Andy Shevchenko wrote:
> > On Wed, Sep 7, 2022 at 6:21 PM Jean Delvare <jdelvare@suse.de> wrote:
> > > On Wed, 7 Sep 2022 17:52:10 +0300, Andy Shevchenko wrote:
...
> > > > "NOTE: This value was incorrectly stated in version 2.1 of this specification as
> > > > 1Eh. Because of this, there might be version 2.1 implementations that
> > > > use either the 1Eh or the 1Fh value, but version 2.2 or later
> > > > implementations must use the 1Fh value."
> > >
> > > Good point, so maybe we should accept 0x1E and treat is silently as
> > > 0x1F (which is what we have been doing implicitly so far) for maximum
> > > compatibility?
> >
> > At least the previous comparison covers this case, if I'm not mistaken.
>
> Before my proposed change, yes. After my proposed change, no longer.
> Let's not risk a regression, I'll change the check to:
>
> if (memcmp(buf, "_SM_", 4) == 0 &&
> buf[5] >= 30 && buf[5] <= 32 &&
> dmi_checksum(buf, buf[5])) {
>
> I'll also add a comment stating why we are allowing length 30.
>
> Thanks for the valuable feedback,
You're welcome! You may add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
to the resulting patch.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-07 16:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-07 8:30 [PATCH] firmware: dmi: Fortify entry point length checks Jean Delvare
2022-09-07 14:52 ` Andy Shevchenko
2022-09-07 15:21 ` Jean Delvare
2022-09-07 15:48 ` Andy Shevchenko
2022-09-07 16:09 ` Jean Delvare
2022-09-07 16:22 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®