diff mbox series

[XEN,v2] x86/apic: Fix signed shift in io_apic.c

Message ID d71b732050d4fff3208205b3117ac5164f889a63.1718897157.git.matthew.barnes@cloud.com (mailing list archive)
State New, archived
Headers show
Series [XEN,v2] x86/apic: Fix signed shift in io_apic.c | expand

Commit Message

Matthew Barnes June 20, 2024, 3:36 p.m. UTC 
There exists bitshifts in the IOAPIC code where signed integers are
shifted to the left by up to 31 bits, which is undefined behaviour.

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

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v2:
- Correct signed shifting in mask_and_ack_level_ioapic_irq()
- Adjust bracket spacing to uphold Xen style
- Improve commit message
---
 xen/arch/x86/io_apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich June 20, 2024, 3:40 p.m. UTC | #1 
On 20.06.2024 17:36, Matthew Barnes wrote:
> There exists bitshifts in the IOAPIC code where signed integers are
> shifted to the left by up to 31 bits, which is undefined behaviour.
> 
> This patch fixes this by changing the integers from signed to unsigned.
> 
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Only almost, ...

> ---
> Changes in v2:
> - Correct signed shifting in mask_and_ack_level_ioapic_irq()
> - Adjust bracket spacing to uphold Xen style

... as that was only half of what I had asked for. The other half was ...

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1692,7 +1692,7 @@ static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
>         !io_apic_level_ack_pending(desc->irq))
>          move_masked_irq(desc);
>  
> -    if ( !(v & (1 << (i & 0x1f))) ) {
> +    if ( !(v & (1U << (i & 0x1f))) ) {
>          spin_lock(&ioapic_lock);
>          __edge_IO_APIC_irq(desc->irq);
>          __level_IO_APIC_irq(desc->irq);
> @@ -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);

... to put each opening figure brace on their own line. I guess Andrew or
I will do that while committing then.

Jan
Andrew Cooper June 20, 2024, 4:09 p.m. UTC | #2 
On 20/06/2024 4:40 pm, Jan Beulich wrote:
> On 20.06.2024 17:36, Matthew Barnes wrote:
>> There exists bitshifts in the IOAPIC code where signed integers are
>> shifted to the left by up to 31 bits, which is undefined behaviour.
>>
>> This patch fixes this by changing the integers from signed to unsigned.
>>
>> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Only almost, ...
>
>> ---
>> Changes in v2:
>> - Correct signed shifting in mask_and_ack_level_ioapic_irq()
>> - Adjust bracket spacing to uphold Xen style
> ... as that was only half of what I had asked for. The other half was ...
>
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -1692,7 +1692,7 @@ static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
>>         !io_apic_level_ack_pending(desc->irq))
>>          move_masked_irq(desc);
>>  
>> -    if ( !(v & (1 << (i & 0x1f))) ) {
>> +    if ( !(v & (1U << (i & 0x1f))) ) {
>>          spin_lock(&ioapic_lock);
>>          __edge_IO_APIC_irq(desc->irq);
>>          __level_IO_APIC_irq(desc->irq);
>> @@ -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);
> ... to put each opening figure brace on their own line. I guess Andrew or
> I will do that while committing then.

Yeah.  That can be fixed on commit.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff mbox series

Patch

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b48a64246548..d9070601a26f 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1692,7 +1692,7 @@  static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
        !io_apic_level_ack_pending(desc->irq))
         move_masked_irq(desc);
 
-    if ( !(v & (1 << (i & 0x1f))) ) {
+    if ( !(v & (1U << (i & 0x1f))) ) {
         spin_lock(&ioapic_lock);
         __edge_IO_APIC_irq(desc->irq);
         __level_IO_APIC_irq(desc->irq);
@@ -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);