RE: [PATCH] firmware: dmi: Stop decoding on broken entry

[Michael Kelley]

From: Jean Delvare <jdelvare@xxxxxxx> Sent: Wednesday, April 17, 2024 10:34 AM
> 
> Hi Michael,
> 
> On Wed, 2024-04-17 at 15:43 +0000, Michael Kelley wrote:
> > From: Jean Delvare <jdelvare@xxxxxxx> Sent: Wednesday, April 17, 2024 8:34 AM
> > >
> > > If a DMI table entry is shorter than 4 bytes, it is invalid. Due to
> > > how DMI table parsing works, it is impossible to safely recover from
> > > such an error, so we have to stop decoding the table.
> > >
> > > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> > > Link: https://lore.kernel.org/linux-kernel/Zh2K3-HLXOesT_vZ@liuwe-devbox-debian-v2/T/
> > > ---
> > > Michael, can you please test this patch and confirm that it prevents
> > > the early oops?
> > >
> > > The root cause of the DMI table corruption still needs to be
> > > investigated.
> > >
> > >  drivers/firmware/dmi_scan.c |   11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > --- linux-6.8.orig/drivers/firmware/dmi_scan.c
> > > +++ linux-6.8/drivers/firmware/dmi_scan.c
> > > @@ -102,6 +102,17 @@ static void dmi_decode_table(u8 *buf,
> > >                 const struct dmi_header *dm = (const struct dmi_header *)data;
> > >
> > >                 /*
> > > +                * If a short entry is found (less than 4 bytes), not only it
> > > +                * is invalid, but we cannot reliably locate the next entry.
> > > +                */
> > > +               if (dm->length < sizeof(struct dmi_header)) {
> > > +                       pr_warn(FW_BUG
> > > +                               "Corrupted DMI table (only %d entries processed)\n",
> > > +                               i);
> >
> > It would be useful to also output the three header fields: type, handle, and length,
> 
> I object. The most likely cause for the length being wrong is memory
> corruption. We have no idea what caused it, nor what kind of data was
> written over the DMI table, so leaking the information to user-space
> doesn't sound like a good idea, even if it's only 4 bytes.
> 
> Furthermore, the data in question is essentially useless anyway. It is
> likely to lead the person investigating the bug into the wrong
> direction by interpreting essentially random data as type, handle and
> length.
> 
> > and perhaps also the offset of the header in the DMI blob (i.e., "data - buf").
> 
> I could do that, as it isn't leaking any information, and this could be
> used to compute the memory address at which the corruption was
> detected, which is probably more useful than the number of the
> corrupted entry. Thanks for the suggestion.
> 
> > When looking at the error reported by user space dmidecode, the first thing
> > I did was add those fields to the error message.
> 
> And this did not give you any further insight, did it?

Agreed.  The offset was probably more useful than the fields from the
header. With the offset, "hexdump /sys/firmware/dmi/tables/DMI"
shows what the bad data looks like.  So if you want to do only the offset,
I'm OK with that.

Michael