Re: [PATCH v3 3/3] lib/firmware_table: Provide buffer length argument to cdat_table_parse()

[Robert Richter <rrichter@amd.com>]

On 14.02.24 17:44:44, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 17:39:27 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Fri, 9 Feb 2024 20:26:47 +0100
> > Robert Richter <rrichter@amd.com> 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@kernel.org>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>  
> > 
> > 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