Re: [XEN PATCH] xen/vpci: Fix UB in mask_write

["Roger Pau Monné" <roger.pau@citrix.com>]

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.