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

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

On Wed, 13 Sep 2023 13:36:46 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Dmitry,
> 
> On 13/9/23 12:10, Dmitry Frolov 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.  
> 
> "statically"
> 
> > Thus, out of bound array access may occur.

If going around again:
Rewrap the above text to be around 75 chars long.
> > 
> > Fixes: c28db9e000 ("hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV")
> > 

No blank line here. Fixes tag is part of the tag block that automated tools
will pick up.

> > Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> > ---
> >   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;  
> 
> The loop in cxl_fixed_memory_window_config() is indeed unsafe.
> 
> OOB can be catched adding:
> 
> -- >8 --  
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 034c7805b3..fe9143409b 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -33,6 +33,7 @@ static void cxl_fixed_memory_window_config(CXLState 
> *cxl_state,
>       for (target = object->targets; target; target = target->next) {
>           fw->num_targets++;
>       }
> +    assert(fw->num_targets <= ARRAY_SIZE(fw->target_hbs));
> 
>       fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
>       if (*errp) {
> ---
> 
> If Jonathan concurs, please add to your patch.
I disagree. 

cxl_interleave_ways_enc() will spit out an error if the value greater
than 16 as well as handling all the other invalid cases as this
can only take values
1,2,3,4,6,8,12,16

Only issue here was that 12 and 16 were values that are accepted but
would overflow the buffer.
Note we don't yet support decoding multiple of 3 interleaves but for
this particular bit of code that isn't a problem.



> 
> Thanks,
> 
> Phil.
>