There exists a bitshift in the IOAPIC code where a signed integer is
shifted to the left by at most 31 bits. This is undefined behaviour,
and can cause faults in xtf tests such as pv64-pv-iopl~hypercall.

This patch fixes this by changing the integer from signed to unsigned.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/arch/x86/io_apic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b48a64246548..ae88b1b898fe 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1756,7 +1756,7 @@ static void cf_check end_level_ioapic_irq_new(struct irq_desc *desc, u8 vector)
          !io_apic_level_ack_pending(desc->irq) )
         move_native_irq(desc);
 
-    if (!(v & (1 << (i & 0x1f)))) {
+    if (!(v & (1U << (i & 0x1f)))) {
         spin_lock(&ioapic_lock);
         __mask_IO_APIC_irq(desc->irq);
         __edge_IO_APIC_irq(desc->irq);
-- 
2.34.1

On 20/06/2024 3:31 pm, Matthew Barnes wrote:
> There exists a bitshift in the IOAPIC code where a signed integer is
> shifted to the left by at most 31 bits. This is undefined behaviour,
> and can cause faults in xtf tests such as pv64-pv-iopl~hypercall.
>
> This patch fixes this by changing the integer from signed to unsigned.
>
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>

The code change itself is fine, but I'm going to recommend some
adjustments to the commit message.

Its "x86/ioapic";  apic implies the Local APIC which is apic.c and
distinct from the IO-APIC.  The subject would be clearer as "Fix signed
shift in end_level_ioapic_irq_new()".

The XTF test has nothing to do with this, so shouldn't be mentioned like
this.  The UBSAN failure was in an interrupt handler, and it was
complete chance that it triggered while pv64-pv-iopl~hypercall was the
test being ran.

I'm happy to fix all of that up on commit.

CC Oleksii for 4.19.  This is low risk, and found during testing with
UBSAN active.

~Andrew

On Thu, 2024-06-20 at 16:16 +0100, Andrew Cooper wrote:
> On 20/06/2024 3:31 pm, Matthew Barnes wrote:
> > There exists a bitshift in the IOAPIC code where a signed integer
> > is
> > shifted to the left by at most 31 bits. This is undefined
> > behaviour,
> > and can cause faults in xtf tests such as pv64-pv-iopl~hypercall.
> > 
> > This patch fixes this by changing the integer from signed to
> > unsigned.
> > 
> > Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
> 
> The code change itself is fine, but I'm going to recommend some
> adjustments to the commit message.
> 
> Its "x86/ioapic";  apic implies the Local APIC which is apic.c and
> distinct from the IO-APIC.  The subject would be clearer as "Fix
> signed
> shift in end_level_ioapic_irq_new()".
> 
> The XTF test has nothing to do with this, so shouldn't be mentioned
> like
> this.  The UBSAN failure was in an interrupt handler, and it was
> complete chance that it triggered while pv64-pv-iopl~hypercall was
> the
> test being ran.
> 
> I'm happy to fix all of that up on commit.
> 
> CC Oleksii for 4.19.  This is low risk, and found during testing with
> UBSAN active.
Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

On 20.06.2024 16:31, Matthew Barnes wrote:
> There exists a bitshift in the IOAPIC code where a signed integer is
> shifted to the left by at most 31 bits. This is undefined behaviour,

s/at most/up to/ maybe?

> and can cause faults in xtf tests such as pv64-pv-iopl~hypercall.
> 
> This patch fixes this by changing the integer from signed to unsigned.
> 
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
> ---
>  xen/arch/x86/io_apic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1756,7 +1756,7 @@ static void cf_check end_level_ioapic_irq_new(struct irq_desc *desc, u8 vector)
>           !io_apic_level_ack_pending(desc->irq) )
>          move_native_irq(desc);
>  
> -    if (!(v & (1 << (i & 0x1f)))) {
> +    if (!(v & (1U << (i & 0x1f)))) {
>          spin_lock(&ioapic_lock);
>          __mask_IO_APIC_irq(desc->irq);
>          __edge_IO_APIC_irq(desc->irq);

For one, can you please also take care of the similar issue in
mask_and_ack_level_ioapic_irq()? And then here and there, can you please
also address the style issue(s) on the line(s) you're touching? In both
cases it will want to be

    if ( !(v & (1U << (i & 0x1f))) )
    {

thus bringing both fully into proper Xen style afaics. Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan