linux-kernel - Re: [PATCH v3 3/3] lib/firmware_table: Provide buffer length argument to cdat_table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 16 Feb 2024 14:07:51 +0100
From: Robert Richter <rrichter@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
	Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v3 3/3] lib/firmware_table: Provide buffer length
 argument to cdat_table_parse()

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