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

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

[Dmitry Frolov]

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>
---
 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;
-- 
2.34.1

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

[Jonathan Cameron via]

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;

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

[Philippe Mathieu-Daudé]

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.
> 
> Fixes: c28db9e000 ("hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV")
> 
> 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.

Thanks,

Phil.

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

[Dmitry Frolov]

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>
---
v2: assert added
---
 hw/cxl/cxl-host.c    | 1 +
 include/hw/cxl/cxl.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

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) {
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;
-- 
2.34.1

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

[Jonathan Cameron via]

On Wed, 13 Sep 2023 16:22:28 +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>
> ---
> v2: assert added
> ---
>  hw/cxl/cxl-host.c    | 1 +
>  include/hw/cxl/cxl.h | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> 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));

Just incase it gets lost as I replied late to the v1 comment.

This is both unnecessary and results in an
assert for a case that would otherwise be a nice error message about unsupported
number of interleave ways.

Jonathan

>  
>      fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
>      if (*errp) {
> 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;

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

[Jonathan Cameron via]

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.
> 

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

[Dmitry Frolov]

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 statically declared of length 8. Thus,
out of bound array access may occur.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

v2: assert added
v3: assert removed

Fixes: c28db9e000 ("hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV")
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;
-- 
2.34.1

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

[Michael Tokarev]

14.09.2023 10:06, 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 statically declared of length 8. Thus,
> out of bound array access may occur.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> v2: assert added
> v3: assert removed

So it's the same as the initial submission.

/mjt

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

[Michael Tokarev]

13.09.2023 13: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.
> 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>

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
(with the extra empty line removed :)

Thanks!

/mjt

> ---
>   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;

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

[Michael Tokarev]

14.09.2023 15:37, Michael Tokarev:
> 13.09.2023 13: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.
>> 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>
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> (with the extra empty line removed :)

Also,

Cc: qemu-stable@nongnu.org

for stable-8.1.

/mjt

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

[Philippe Mathieu-Daudé]

On 14/9/23 14:38, Michael Tokarev wrote:
> 14.09.2023 15:37, Michael Tokarev:
>> 13.09.2023 13: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.
>>> 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>
>>
>> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>> (with the extra empty line removed :)
> 
> Also,
> 
> Cc: qemu-stable@nongnu.org
> 
> for stable-8.1.

[not related to this particular patch]

Maybe this can help if we specify the releases range as a comment
in the Cc tag, for example here:

Cc: qemu-stable@nongnu.org # v8.1

and if it were a range:

Cc: qemu-stable@nongnu.org # v6.2 to v8.0

Michael would that help? If so feel free to modify
docs/devel/stable-process.rst :)

Regards,

Phil.

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

[Michael Tokarev]

14.09.2023 15:59, Philippe Mathieu-Daudé wrote:

>> Cc: qemu-stable@nongnu.org
>> for stable-8.1.
> 
> [not related to this particular patch]
> 
> Maybe this can help if we specify the releases range as a comment
> in the Cc tag, for example here:
> 
> Cc: qemu-stable@nongnu.org # v8.1
> 
> and if it were a range:
> 
> Cc: qemu-stable@nongnu.org # v6.2 to v8.0
> 
> Michael would that help? If so feel free to modify
> docs/devel/stable-process.rst :)

I don't think this is necessary so far.

Or, actually it might help for sure, but it is an extra burden
for regular developers.

For quite some things I can see where it is applicable if there's
a proper Fixes: tag already (that's why I've added this Cc), - it's
trivial to run `git describe' for this commit ID, so it doesn't even
need to have a Cc: stable.

In some cases though it is hard to tell how deep a change needs to
be backported. In such cases some hint can help for sure, but when
in doubt I can ask too.  When you're in context of fixing something,
you usually don't want to think about how to backport it or somesuch,
you concentrate on the fix itself. If you're willing to think about
that too, give a small note in the comment, like some authors do.
If not, and it's entirely okay, and it's unclear if the change should
be applied to earlier versions, I can ask (or notify when picking the
change up for stable), and the author might think about this in another
context.  It's not often - so far anyway - when it's unclear if a
change should be propagated to older releases.

Changing stable-process.rst isn't very useful, as most changes are
changing master, - if anything, regular "contribution" docs should
be changed for that.  But since most stuff is going on through various
subsystem maintainers, they don't usually look there anyway :)

For now, it's basically my own whim to provide stable series for
older qemu releases.  Or an experiment.  I don't know how useful it
is (or will be) and how it will go long-term.  We've never had this
before.

In my view it is much more important to either add the Fixes: tag
(which gives me a hint already, as I check for all Fixes: in patches
being applied, and can automate this further by doing git-desc on
the hashes, alerting me if it is before the current release) or realize
something needs to go to stable at all. Even at the cost of sending
extra stuff to stable which is actually not needed at all - this is
entirely okay.  This is why I'm asking about various changes going
in, reminding about -stable existance, - because some stuff isn't
marked as a fix at all but in fact it is an important thing for
stable (one or another).

Thank you for your support Phil! :)

/mjt