linux-kernel - Re: [PATCH v5] 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: Mon, 19 Feb 2024 12:53:34 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Richter <rrichter@....com>
CC: Dan Williams <dan.j.williams@...el.com>, kernel test robot
	<lkp@...el.com>, Alison Schofield <alison.schofield@...el.com>, "Vishal
 Verma" <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...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>,
	<oe-kbuild-all@...ts.linux.dev>, Linux Memory Management List
	<linux-mm@...ck.org>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Len Brown <lenb@...nel.org>,
	<linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v5] lib/firmware_table: Provide buffer length argument
 to cdat_table_parse()

On Sat, 17 Feb 2024 22:39:46 +0100
Robert Richter <rrichter@....com> wrote:

> On 17.02.24 18:43:37, kernel test robot wrote:
> > Hi Robert,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > [auto build test WARNING on 6be99530c92c6b8ff7a01903edc42393575ad63b]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Robert-Richter/cxl-pci-Rename-DOE-mailbox-handle-to-doe_mb/20240217-000206
> > base:   6be99530c92c6b8ff7a01903edc42393575ad63b
> > patch link:    https://lore.kernel.org/r/20240216155844.406996-4-rrichter%40amd.com
> > patch subject: [PATCH v4 3/3] lib/firmware_table: Provide buffer length argument to cdat_table_parse()
> > config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20240217/202402171817.i0WShbft-lkp@intel.com/config)
> > compiler: arceb-elf-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240217/202402171817.i0WShbft-lkp@intel.com/reproduce)  
> 
> >    In file included from include/linux/device.h:15,
> >                     from drivers/cxl/core/pci.c:5:
> >    drivers/cxl/core/pci.c: In function 'read_cdat_data':  
> > >> drivers/cxl/core/pci.c:672:31: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]  
> >      672 |                 dev_warn(dev, "Malformed CDAT table length (%lu:%lu), discarding trailing data\n",
> >          |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  
> 
> Fix below, it basically uses %zu for both format strings.
> 
> -Robert
> 
> 
> From 08685053a91e370fd1263b921aa3e8942025c4e4 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@....com>
> Date: Sun, 7 Jan 2024 18:13:16 +0100
> Subject: [PATCH v5] lib/firmware_table: Provide buffer length argument to
>  cdat_table_parse()
> 
> There exist card implementations with a CDAT table using a fixed size
> 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.
> 
> Add a check to warn about a malformed CDAT table length.
> 
> 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>
> Signed-off-by: Robert Richter <rrichter@....com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>