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
