Re: [PATCH v3 02/16] cxl/pci: Handle truncated CDAT header

[Lukas Wunner <lukas@wunner.de>]

On Fri, Feb 10, 2023 at 04:40:15PM -0800, Dan Williams wrote:
> Lukas Wunner wrote:
> > cxl_cdat_get_length() only checks whether the DOE response size is
> > sufficient for the Table Access response header (1 dword), but not the
> > succeeding CDAT header (1 dword length plus other fields).
> > 
> > It thus returns whatever uninitialized memory happens to be on the stack
> > if a truncated DOE response with only 1 dword was received.  Fix it.
[...]
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -528,7 +528,7 @@ static int cxl_cdat_get_length(struct device *dev,
> >  		return rc;
> >  	}
> >  	wait_for_completion(&t.c);
> > -	if (t.task.rv < sizeof(u32))
> > +	if (t.task.rv < 2 * sizeof(u32))
> >  		return -EIO;
> 
> Looks good, I wonder since this is standard for all data objects whether
> the check should be pushed into the core?

No, "t.task.rv" contains the payload length in bytes of the response
received via DOE.  It doesn't include the DOE header.

I think it is legal to receive an empty response via DOE, so I cannot
push a length check down into the core.

In this case, the payload contains one dword for the Table Access Response
header (CXL r3.0 sec 8.1.11.1), followed by 3 dwords for the CDAT header:

https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf

The CDAT header contains the length of the entire CDAT in the first
dword, hence the above-quoted check only verifies that at least two
dwords were received.  It's harmless if the remainder of the CDAT
header is truncated.

Thanks,

Lukas