On 14.02.24 17:44:44, Jonathan Cameron wrote: > On Wed, 14 Feb 2024 17:39:27 +0000 > Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote: > > > On Fri, 9 Feb 2024 20:26:47 +0100 > > Robert Richter <rrichter@xxxxxxx> wrote: > > > > > There exists card implementations with a CDAT table using a fix > > There exist ... fixed size buffer, > > > buffer, but with entries filled in that do not fill the whole table > > > length size. Then, the last entry in the CDAT table may not mark the > > > end of the CDAT table buffer specified by the length field in the CDAT > > > header. It can be shorter with trailing unused (zero'ed) data. The > > > actual table length is determined while reading all CDAT entries of > > > the table with DOE. > > > > > > If the table is greater than expected (containing zero'ed trailing > > > data), the CDAT parser fails with: > > > > > > [ 48.691717] Malformed DSMAS table length: (24:0) > > > [ 48.702084] [CDAT:0x00] Invalid zero length > > > [ 48.711460] cxl_port endpoint1: Failed to parse CDAT: -22 > > > > > > In addition, a check of the table buffer length is missing to prevent > > > an out-of-bound access then parsing the CDAT table. > > > > > > Hardening code against device returning borked table. Fix that by > > > providing an optional buffer length argument to > > > acpi_parse_entries_array() that can be used by cdat_table_parse() to > > > propagate the buffer size down to its users to check the buffer > > > length. This also prevents a possible out-of-bound access mentioned. > > > > > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx> > > > Cc: Len Brown <lenb@xxxxxxxxxx> > > > Signed-off-by: Robert Richter <rrichter@xxxxxxx> > > > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx> > > > > I think we should scream a bit about this if we see it > > as I'm unconvinced the spec allows for an implementation like this. > > > > If the spec is unclear, lets seek a clarification. > > > > I'm fine with this as a defensive measure, I just don't want > > device vendors to keep doing it! > > > Scrub that - I got around to checking the CDAT spec. It can > change length whilst we are reading it due to DSEMTS entry > counts being allowed to change. > https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.03.pdf > (it's in the description of the Sequence field) > > Sure we'll notice that the checksum fails and the sequence number > has updated but that doesn't help us if we went out of bounds > before we knew that. > > Definitely good to check this as I think we can hit it even > if we don't have a potentially buggy device. > I'd still like to moan if we get inconsistent sizes and it > isn't a race though. Can we find a clean way to detect this > at a point where we know we have a valid complete table? I will add a warning about a length mismatch. Thanks, -Robert
