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