| Message ID | 20230913101055.754709-1-frolov@swemel.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/cxl: Fix out of bound array access | expand |
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;
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.
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. >
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;
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
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.
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
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;
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(-)