Re: [PATCH v3 03/16] cxl/pci: Handle truncated CDAT entries

[Lukas Wunner <lukas@wunner.de>]

On Fri, Feb 10, 2023 at 04:50:38PM -0800, Dan Williams wrote:
> Lukas Wunner wrote:
> > If truncated CDAT entries are received from a device, the concatenation
> > of those entries constitutes a corrupt CDAT, yet is happily exposed to
> > user space.
> > 
> > Avoid by verifying response lengths and erroring out if truncation is
> > detected.
[...]
> > @@ -557,14 +557,19 @@ static int cxl_cdat_read_table(struct device *dev,
> >  			return rc;
> >  		}
> >  		wait_for_completion(&t.c);
> > -		/* 1 DW header + 1 DW data min */
> > -		if (t.task.rv < (2 * sizeof(u32)))
> 
> Ah, I guess that's why the previous check can not be pushed down
> further, its obviated by this more comprehensive check.

The check in the previous patch ("cxl/pci: Handle truncated CDAT header")
fixes response size verification for cxl_cdat_get_length().

The check here however is in a different function, cxl_cdat_read_table().
Previously the function only checked whether the DOE response contained
two dwords.  That's one dword for the Table Access Response header
(CXL r3.0 sec 8.1.11.1) plus one dword for the common header shared
by all CDAT structures.

What can happen is we receive a truncated entry with, say, just
two dwords, and we copy that to the cached CDAT exposed in sysfs.
Obviously that's a corrupt CDAT.  The consumer of the CDAT doesn't
know that the data was truncated and will try to parse the entry
even though the data may actually be a portion of the next entry.

The improved verification here ensures that the received amount of
data corresponds to the length in the entry header.

It is up to the parsing function of each entry to verify whether
that length is correct for that specific entry.  E.g. a DSMAS entry
should contain 24 bytes, so the parsing function needs to verify
that the length in the entry header is actually 24.  (See the e-mail
I sent to Dave a few minutes ago.)

Thanks,

Lukas

> > +		/* 1 DW Table Access Response Header + CDAT entry */
> > +		entry = (struct cdat_entry_header *)(t.response_pl + 1);
> > +		if ((entry_handle == 0 &&
> > +		     t.task.rv != sizeof(u32) + sizeof(struct cdat_header)) ||
> > +		    (entry_handle > 0 &&
> > +		     (t.task.rv < sizeof(u32) + sizeof(struct cdat_entry_header) ||
> > +		      t.task.rv != sizeof(u32) + le16_to_cpu(entry->length))))
> >  			return -EIO;