[PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[Andrew Cooper]

The MP spec requires a minimum of 1k alignment.  So too does Xen's use of a
single fixmap page to map the IO-APIC.

Reject out-of-spec values so we don't end up in a position where a bad
firmware value causes Xen to use the wrong mapping.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/io_apic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 0ef61fb2f167..db1ea6e2f2ca 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void)
         union IO_APIC_reg_01 reg_01;
         paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr;
 
-        if ( !ioapic_phys )
+        if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) )
         {
             printk(KERN_ERR
-                   "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n");
+                   "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n",
+                   ioapic_phys);
             smp_found_config = false;
             skip_ioapic_setup = true;
             break;
-- 
2.30.2

Re: [PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[Jan Beulich]

On 20.11.2023 16:10, Andrew Cooper wrote:
> The MP spec requires a minimum of 1k alignment.  So too does Xen's use of a
> single fixmap page to map the IO-APIC.
> 
> Reject out-of-spec values so we don't end up in a position where a bad
> firmware value causes Xen to use the wrong mapping.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm curious though: Was this observed in practice?

Jan

Re: [PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[Andrew Cooper]

On 20/11/2023 4:06 pm, Jan Beulich wrote:
> On 20.11.2023 16:10, Andrew Cooper wrote:
>> The MP spec requires a minimum of 1k alignment.  So too does Xen's use of a
>> single fixmap page to map the IO-APIC.
>>
>> Reject out-of-spec values so we don't end up in a position where a bad
>> firmware value causes Xen to use the wrong mapping.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I'm curious though: Was this observed in practice?

No.  I had some prototype cleanup which collided with MISRA, and I
realised I hadn't posted this part of the series.

The other part of the cleanup still has work to do, and I don't have
time to unpick it right now.

~Andrew

Re: [PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[Roger Pau Monné]

On Mon, Nov 20, 2023 at 03:10:06PM +0000, Andrew Cooper wrote:
> The MP spec requires a minimum of 1k alignment.  So too does Xen's use of a
> single fixmap page to map the IO-APIC.
> 
> Reject out-of-spec values so we don't end up in a position where a bad
> firmware value causes Xen to use the wrong mapping.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/io_apic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 0ef61fb2f167..db1ea6e2f2ca 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void)
>          union IO_APIC_reg_01 reg_01;
>          paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr;
>  
> -        if ( !ioapic_phys )
> +        if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) )

Might be clearer to use !IS_ALIGNED(), otherwise:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

>          {
>              printk(KERN_ERR
> -                   "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n");
> +                   "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n",

FWIW, I think the '!' at the end is not useful, and I would adjust
the last 'IO/APIC' to 'IO-APIC' if already modifying the line.

Thanks, Roger.

Re: [PATCH] x86/ioapic: Check 1k alignment for IO-APIC physical addresses

[Andrew Cooper]

On 20/11/2023 5:36 pm, Roger Pau Monné wrote:
> On Mon, Nov 20, 2023 at 03:10:06PM +0000, Andrew Cooper wrote:
>> The MP spec requires a minimum of 1k alignment.  So too does Xen's use of a
>> single fixmap page to map the IO-APIC.
>>
>> Reject out-of-spec values so we don't end up in a position where a bad
>> firmware value causes Xen to use the wrong mapping.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/io_apic.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index 0ef61fb2f167..db1ea6e2f2ca 100644
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void)
>>          union IO_APIC_reg_01 reg_01;
>>          paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr;
>>  
>> -        if ( !ioapic_phys )
>> +        if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) )
> Might be clearer to use !IS_ALIGNED()

Hmm, yeah.  Will change.

> , otherwise:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>
>>          {
>>              printk(KERN_ERR
>> -                   "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n");
>> +                   "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n",
> FWIW, I think the '!' at the end is not useful, and I would adjust
> the last 'IO/APIC' to 'IO-APIC' if already modifying the line.

Ok.  will do.

~Andrew