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

[Jonathan Cameron]

On Sat, 17 Feb 2024 22:39:46 +0100
Robert Richter <rrichter@xxxxxxx> 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@xxxxxxxxx/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@xxxxxxxxx/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@xxxxxxx>
> 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@xxxxxxxxxx>
> Cc: Len Brown <lenb@xxxxxxxxxx>
> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Signed-off-by: Robert Richter <rrichter@xxxxxxx>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>