linux-kernel - RE: [PATCH v2 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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Jan 2024 16:32:33 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Robert Richter <rrichter@....com>, 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>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, Dave Jiang
	<dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>, "Rafael J.
 Wysocki" <rafael@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Len Brown
	<lenb@...nel.org>, Robert Richter <rrichter@....com>,
	<linux-acpi@...r.kernel.org>
Subject: RE: [PATCH v2 3/3] lib/firmware_table: Provide buffer length
 argument to cdat_table_parse()

Robert Richter wrote:
> 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 when 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, the table buffer size can be different from the size
> specified in the length field. This may cause out-of-bound access then
> parsing the CDAT table.
> 
> Fix that by providing an optonal 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.
> 
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> Cc: Len Brown <lenb@...nel.org>
> Signed-off-by: Robert Richter <rrichter@....com>
[..]
> diff --git a/lib/fw_table.c b/lib/fw_table.c
> index 1e5e0b2f7012..ddb67853b7ac 100644
> --- a/lib/fw_table.c
> +++ b/lib/fw_table.c
[..]
> @@ -164,8 +166,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>  	int i;
>  
>  	type = acpi_get_subtable_type(id);
> -	table_end = (unsigned long)table_header +
> -		    acpi_table_get_length(type, table_header);
> +	table_len = acpi_table_get_length(type, table_header);
> +	if (max_length && max_length < table_len)
> +		table_len = max_length;

The other patches in this series look good, my only quibble here is
that this is an open-coded min_not_zero(). If nothing else pops up in
testing that triggers a v3 I will just fix that up locally.

Thanks, Robert!