| Message ID | 559dfac91b8f097bc59c4de194fd2ae2b5b4144c.1730880005.git.mykyta_poturai@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] xen/vpci: Fix UB in mask_write | expand |
On Wed, Nov 06, 2024 at 08:05:19AM +0000, Mykyta Poturai wrote: > During the construction of dmask value, it gets shifted by > (32 - msi->vectors) bits. If msi->vectors is 0, the result of the shift > becomes undefined due to shifting by a size of the type. While this > works fine on x86, on ARM the resulting mask becomes 0xFFFFFFFF, which > is incorrect. > > Fix this by adding an explicit check for msi->vectors == 0. I would also add: Fixes: 188fa82305e7 ('xen/vpci: Improve code generation in mask_write()') > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com> > --- > xen/drivers/vpci/msi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c > index 7bda47e7fc..787296fd42 100644 > --- a/xen/drivers/vpci/msi.c > +++ b/xen/drivers/vpci/msi.c > @@ -172,7 +172,7 @@ static void cf_check mask_write( > struct vpci_msi *msi = data; > uint32_t dmask = msi->mask ^ val; > > - if ( !dmask ) > + if ( !dmask || msi->vectors == 0 ) > return; I'm afraid returning this early is not correct - the cached mask needs to be updated, even if there are no vectors currently enabled. The adjustment likely needs to be: if ( msi->enabled && msi->vectors ) ... So that the update of msi->mask is not skipped. Thanks, Roger.
On Wed, Nov 06, 2024 at 10:00:07AM +0100, Roger Pau Monné wrote: > On Wed, Nov 06, 2024 at 08:05:19AM +0000, Mykyta Poturai wrote: > > During the construction of dmask value, it gets shifted by > > (32 - msi->vectors) bits. If msi->vectors is 0, the result of the shift > > becomes undefined due to shifting by a size of the type. While this > > works fine on x86, on ARM the resulting mask becomes 0xFFFFFFFF, which > > is incorrect. > > > > Fix this by adding an explicit check for msi->vectors == 0. Wait - how can msi->vectors ever be 0? AFAICT there's no way in the MSI logic to configure 0 vectors, there will always be at least 1 vector enabled. Maybe what you want, if this fix is for compliance reasons, is an assert unreachable that msi->vectors > 0? Thanks, Roger.
On 06.11.2024 10:07, Roger Pau Monné wrote: > On Wed, Nov 06, 2024 at 10:00:07AM +0100, Roger Pau Monné wrote: >> On Wed, Nov 06, 2024 at 08:05:19AM +0000, Mykyta Poturai wrote: >>> During the construction of dmask value, it gets shifted by >>> (32 - msi->vectors) bits. If msi->vectors is 0, the result of the shift >>> becomes undefined due to shifting by a size of the type. While this >>> works fine on x86, on ARM the resulting mask becomes 0xFFFFFFFF, which >>> is incorrect. >>> >>> Fix this by adding an explicit check for msi->vectors == 0. > > Wait - how can msi->vectors ever be 0? AFAICT there's no way in the > MSI logic to configure 0 vectors, there will always be at least 1 vector > enabled. > > Maybe what you want, if this fix is for compliance reasons, is an > assert unreachable that msi->vectors > 0? Which raises a question as to (lack of) context: Was this spotted by mere code inspection? Or by a static analyzer? If so, which one? That may help figure whether some workaround like the one suggested is necessary, or whether it can simply be left alone. Jan
On 06.11.2024 09:05, Mykyta Poturai wrote: > During the construction of dmask value, it gets shifted by > (32 - msi->vectors) bits. If msi->vectors is 0, the result of the shift > becomes undefined due to shifting by a size of the type. While this > works fine on x86, Oh, also - what made you think this would be fine on x86? Afaict ... > on ARM the resulting mask becomes 0xFFFFFFFF, which > is incorrect. ... the exact same thing would happen (if msi->vectors indeed could ever be zero) there, due to the type of the value shifted being unsigned int, not unsigned long. Jan
On 06.11.2024 10:07, Roger Pau Monné wrote:> > Wait - how can msi->vectors ever be 0? AFAICT there's no way in the > MSI logic to configure 0 vectors, there will always be at least 1 vector > enabled. > > Maybe what you want, if this fix is for compliance reasons, is an > assert unreachable that msi->vectors > 0? I did some investigation and figured out that the value of 0 is being set by guest writing to msi_control_reg. As far as I understand, the control_write() function only checks that vectors are not greater than the maximum allowed value, but does not check for 0. So I am not sure if this is a valid scenario or not. Is this incorrect guest behavior and it should be forbidden from setting vectors to 0 and enable to 1 at the same time? On 06.11.24 13:31, Jan Beulich wrote: > > Which raises a question as to (lack of) context: Was this spotted by > mere code inspection? Or by a static analyzer? If so, which one? That > may help figure whether some workaround like the one suggested is > necessary, or whether it can simply be left alone. > > Jan I have found this while porting the PCI passthrough patches to Xen 4.20. After checking the previous version which was on 4.18 it seems that on it msi->vectors are also set to 0 but nothing breaks due to it being the explicit end of the loop. So I have assumed that setting it to 0 is a valid scenario. I am testing all of this on Rcar Gen4 boards. Mykyta
On Wed, Nov 06, 2024 at 12:26:55PM +0000, Mykyta Poturai wrote: > On 06.11.2024 10:07, Roger Pau Monné wrote:> > > Wait - how can msi->vectors ever be 0? AFAICT there's no way in the > > MSI logic to configure 0 vectors, there will always be at least 1 vector > > enabled. > > > > Maybe what you want, if this fix is for compliance reasons, is an > > assert unreachable that msi->vectors > 0? > > I did some investigation and figured out that the value of 0 is being > set by guest writing to msi_control_reg. As far as I understand, the > control_write() function only checks that vectors are not greater than > the maximum allowed value, but does not check for 0. control_write() will set vectors to (1UL << val), so even if user provides val == 0, vectors will be 1. Can you provide an example input value of control_write() that will lead to msi->vectors == 0? Is maybe msi_maxvec not set correctly in your use case if you indeed see vectors == 0? Thanks, Roger.
On 06.11.24 14:42, Roger Pau Monné wrote: > On Wed, Nov 06, 2024 at 12:26:55PM +0000, Mykyta Poturai wrote: >> On 06.11.2024 10:07, Roger Pau Monné wrote: >>> Wait - how can msi->vectors ever be 0? AFAICT there's no way in the >>> MSI logic to configure 0 vectors, there will always be at least 1 vector >>> enabled. >>> >>> Maybe what you want, if this fix is for compliance reasons, is an >>> assert unreachable that msi->vectors > 0? >> >> I did some investigation and figured out that the value of 0 is being >> set by guest writing to msi_control_reg. As far as I understand, the >> control_write() function only checks that vectors are not greater than >> the maximum allowed value, but does not check for 0. > > control_write() will set vectors to (1UL << val), so even if user > provides val == 0, vectors will be 1. > > Can you provide an example input value of control_write() that will > lead to msi->vectors == 0? > > Is maybe msi_maxvec not set correctly in your use case if you indeed > see vectors == 0? > > Thanks, Roger. Indeed, I have checked and msi_maxvec is set to 0. Thanks for pointing this out. I will investigate further why this is happening. It is quite strange that it somehow worked on 4.18 with the same problem. I will change the check to an assert then, so if something similar happens again it can be caught earlier. Mykyta
On Wed, Nov 06, 2024 at 02:32:13PM +0000, Mykyta Poturai wrote: > On 06.11.24 14:42, Roger Pau Monné wrote: > > On Wed, Nov 06, 2024 at 12:26:55PM +0000, Mykyta Poturai wrote: > >> On 06.11.2024 10:07, Roger Pau Monné wrote: > >>> Wait - how can msi->vectors ever be 0? AFAICT there's no way in the > >>> MSI logic to configure 0 vectors, there will always be at least 1 vector > >>> enabled. > >>> > >>> Maybe what you want, if this fix is for compliance reasons, is an > >>> assert unreachable that msi->vectors > 0? > >> > >> I did some investigation and figured out that the value of 0 is being > >> set by guest writing to msi_control_reg. As far as I understand, the > >> control_write() function only checks that vectors are not greater than > >> the maximum allowed value, but does not check for 0. > > > > control_write() will set vectors to (1UL << val), so even if user > > provides val == 0, vectors will be 1. > > > > Can you provide an example input value of control_write() that will > > lead to msi->vectors == 0? > > > > Is maybe msi_maxvec not set correctly in your use case if you indeed > > see vectors == 0? > > > > Thanks, Roger. > > Indeed, I have checked and msi_maxvec is set to 0. Thanks for pointing > this out. I will investigate further why this is happening. It is quite > strange that it somehow worked on 4.18 with the same problem. Check whether pdev_msi_init() is called during device addition, as that's what initializes msi_maxvec. Another cause could be memory corruption. > I will change the check to an assert then, so if something similar > happens again it can be caught earlier. Let's try to figure out what causes msi_maxvec to be 0 in your case and then we can see how to better detect this. If msi_maxvec needs to be checked it should likely be done in init_msi(). Regards, Roger.
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index 7bda47e7fc..787296fd42 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -172,7 +172,7 @@ static void cf_check mask_write( struct vpci_msi *msi = data; uint32_t dmask = msi->mask ^ val; - if ( !dmask ) + if ( !dmask || msi->vectors == 0 ) return; if ( msi->enabled )
During the construction of dmask value, it gets shifted by (32 - msi->vectors) bits. If msi->vectors is 0, the result of the shift becomes undefined due to shifting by a size of the type. While this works fine on x86, on ARM the resulting mask becomes 0xFFFFFFFF, which is incorrect. Fix this by adding an explicit check for msi->vectors == 0. Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com> --- xen/drivers/vpci/msi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)