| Message ID | c29ced52-6e1e-4997-81ab-8882df2d38a7@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Dom0 I/O port access permissions | expand |
On 2023-12-18 15:48, Jan Beulich wrote: > ... in order to also deny Dom0 access through the alias ports. Without > this it is only giving the impression of denying access to both PICs. > Unlike for CMOS/RTC, do detection very early, to avoid disturbing > normal > operation later on. > > Like for CMOS/RTC a fundamental assumption of the probing is that reads > from the probed alias port won't have side effects in case it does not > alias the respective PIC's one. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Use new command line option. s/pic/8252A/. Re-base over new earlier > patch. Use ISOLATE_LSB(). > Hi, coming back to this patch, which I believe didn't receive much feedback and thus wasn't committed, for a reason: > --- a/xen/arch/x86/i8259.c > +++ b/xen/arch/x86/i8259.c > @@ -19,6 +19,7 @@ > #include <xen/delay.h> > #include <asm/apic.h> > #include <asm/asm_defns.h> > +#include <asm/setup.h> Here asm/setup is included, which provides the declaration for init_IRQ, defined below in the file > > void __init init_IRQ(void) > @@ -343,6 +396,8 @@ void __init init_IRQ(void) > which is defined here. This patch would, among other things, address a MISRA C Rule 8.4 violation ("A compatible declaration shall be visible when an object or function with external linkage is defined"). I did send a patch concerned only with the MISRA violation, but correctly it was pointed out that this one was doing that and more. Perhaps someone can have a look at this? > init_8259A(0); > > + probe_8259A_alias(); > + > for (irq = 0; platform_legacy_irq(irq); irq++) { > struct irq_desc *desc = irq_to_desc(irq); > > --- a/xen/arch/x86/include/asm/setup.h > +++ b/xen/arch/x86/include/asm/setup.h > @@ -46,6 +46,8 @@ extern uint8_t kbd_shift_flags; > extern unsigned long highmem_start; > #endif > > +extern unsigned int i8259A_alias_mask; > + > extern int8_t opt_smt; > extern int8_t opt_probe_port_aliases;
On 2023-12-18 09:48, Jan Beulich wrote: > ... in order to also deny Dom0 access through the alias ports. Without > this it is only giving the impression of denying access to both PICs. > Unlike for CMOS/RTC, do detection very early, to avoid disturbing normal > operation later on. > > Like for CMOS/RTC a fundamental assumption of the probing is that reads > from the probed alias port won't have side effects in case it does not > alias the respective PIC's one. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Code-wise Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> I appreciate what Jan is trying to achieve. I also share Roger's concern about reading IO ports since we don't know what is there. I also don't have a good alternative to suggest... Regards, Jason
--- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -467,7 +467,7 @@ static void __init process_dom0_ioports_ int __init dom0_setup_permissions(struct domain *d) { unsigned long mfn; - unsigned int i; + unsigned int i, offs; int rc; if ( pv_shim ) @@ -480,10 +480,16 @@ int __init dom0_setup_permissions(struct /* Modify I/O port access permissions. */ - /* Master Interrupt Controller (PIC). */ - rc |= ioports_deny_access(d, 0x20, 0x21); - /* Slave Interrupt Controller (PIC). */ - rc |= ioports_deny_access(d, 0xA0, 0xA1); + for ( offs = 0, i = ISOLATE_LSB(i8259A_alias_mask) ?: 2; + offs <= i8259A_alias_mask; offs += i ) + { + if ( offs & ~i8259A_alias_mask ) + continue; + /* Master Interrupt Controller (PIC). */ + rc |= ioports_deny_access(d, 0x20 + offs, 0x21 + offs); + /* Slave Interrupt Controller (PIC). */ + rc |= ioports_deny_access(d, 0xA0 + offs, 0xA1 + offs); + } /* ELCR of both PICs. */ rc |= ioports_deny_access(d, 0x4D0, 0x4D1); --- a/xen/arch/x86/i8259.c +++ b/xen/arch/x86/i8259.c @@ -19,6 +19,7 @@ #include <xen/delay.h> #include <asm/apic.h> #include <asm/asm_defns.h> +#include <asm/setup.h> #include <io_ports.h> #include <irq_vectors.h> @@ -333,6 +334,58 @@ void __init make_8259A_irq(unsigned int irq_to_desc(irq)->handler = &i8259A_irq_type; } +unsigned int __initdata i8259A_alias_mask; + +static void __init probe_8259A_alias(void) +{ + unsigned int mask = 0x1e; + uint8_t val = 0; + + if ( !opt_probe_port_aliases ) + return; + + /* + * The only properly r/w register is OCW1. While keeping the master + * fully masked (thus also masking anything coming through the slave), + * write all possible 256 values to the slave's base port, and check + * whether the same value can then be read back through any of the + * possible alias ports. Probing just the slave of course builds on the + * assumption that aliasing is identical for master and slave. + */ + + outb(0xff, 0x21); /* Fully mask master. */ + + do { + unsigned int offs; + + outb(val, 0xa1); + + /* Try to make sure we're actually having a PIC here. */ + if ( inb(0xa1) != val ) + { + mask = 0; + break; + } + + for ( offs = ISOLATE_LSB(mask); offs <= mask; offs <<= 1 ) + { + if ( !(mask & offs) ) + continue; + if ( inb(0xa1 + offs) != val ) + mask &= ~offs; + } + } while ( mask && (val += 0x0d) ); /* Arbitrary uneven number. */ + + outb(cached_A1, 0xa1); /* Restore slave IRQ mask. */ + outb(cached_21, 0x21); /* Restore master IRQ mask. */ + + if ( mask ) + { + dprintk(XENLOG_INFO, "PIC aliasing mask: %02x\n", mask); + i8259A_alias_mask = mask; + } +} + static struct irqaction __read_mostly cascade = { no_action, "cascade", NULL}; void __init init_IRQ(void) @@ -343,6 +396,8 @@ void __init init_IRQ(void) init_8259A(0); + probe_8259A_alias(); + for (irq = 0; platform_legacy_irq(irq); irq++) { struct irq_desc *desc = irq_to_desc(irq); --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -46,6 +46,8 @@ extern uint8_t kbd_shift_flags; extern unsigned long highmem_start; #endif +extern unsigned int i8259A_alias_mask; + extern int8_t opt_smt; extern int8_t opt_probe_port_aliases;
... in order to also deny Dom0 access through the alias ports. Without this it is only giving the impression of denying access to both PICs. Unlike for CMOS/RTC, do detection very early, to avoid disturbing normal operation later on. Like for CMOS/RTC a fundamental assumption of the probing is that reads from the probed alias port won't have side effects in case it does not alias the respective PIC's one. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Use new command line option. s/pic/8252A/. Re-base over new earlier patch. Use ISOLATE_LSB().