Re: [PATCH] hw/cxl: Fix out of bound array access

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Wed, 13 Sep 2023 13:10:56 +0300
Dmitry Frolov <frolov@swemel.ru> wrote:

> According to cxl_interleave_ways_enc(),
> fw->num_targets is allowed to be up to 16.
> This also corresponds to CXL specs.
> So, the fw->target_hbs[] array is iterated from 0 to 15.
> But it is staticaly declared of length 8.
> Thus, out of bound array access may occur.
> 
> Fixes: c28db9e000 ("hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV")
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>

Hi Dmitry,

Good spot - though I'm curious on whether you hit this in a 16 way interleave test and
hence care about this case?  My tests tend to burn the available ways in the topology
rather than doing a flat 16 way host interleave (which would be a crazy physical system
- I want one of those :)

This looks to be a missed update when we expanded the decoded number of interleave ways.
I think (looking at published ECNs) that occurred in a CXL r2.0 ECN dated Oct 2021.
The CFWMS table was introduced as an ECN published in May 2021.  I'll note the r3.0
spec is confusing because CFMWS refers to the HDM decoder spec that says the values
beyond 1,2,4,8 are for endpoints only and this isn't one.  Examples make it clear
that rule doesn't apply though.

I suspect this bug was introduced whilst the code was still out of tree so hard to point at
when.

Anyhow, I'll queue this one or Michael can pick it up directly if he'd prefer.

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


> ---
>  include/hw/cxl/cxl.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index 56c9e7676e..4944725849 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -29,7 +29,7 @@ typedef struct PXBCXLDev PXBCXLDev;
>  typedef struct CXLFixedWindow {
>      uint64_t size;
>      char **targets;
> -    PXBCXLDev *target_hbs[8];
> +    PXBCXLDev *target_hbs[16];
>      uint8_t num_targets;
>      uint8_t enc_int_ways;
>      uint8_t enc_int_gran;