On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
> On 26.10.2023 12:25, Roger Pau Monné wrote:
> > On Thu, May 11, 2023 at 02:07:12PM +0200, 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 PIT. Unlike
> >> for CMOS/RTC, do detection pretty early, to avoid disturbing normal
> >> operation later on (even if typically we won't use much of the PIT).
> >>
> >> Like for CMOS/RTC a fundamental assumption of the probing is that reads
> >> from the probed alias port won't have side effects (beyond such that PIT
> >> reads have anyway) in case it does not alias the PIT's.
> >>
> >> At to the port 0x61 accesses: Unlike other accesses we do, this masks
> >> off the top four bits (in addition to the bottom two ones), following
> >> Intel chipset documentation saying that these (read-only) bits should
> >> only be written with zero.
> >
> > As said in previous patches, I think this is likely too much risk for
> > little benefit. I understand the desire to uniformly deny access to
> > any ports that allow interaction with devices in use by Xen (or not
> > allowed to be used by dom0), but there's certainly a risk in
> > configuring such devices in the way that we do by finding a register
> > that can be read and written to.
> >
> > I think if anything this alias detection should have a command line
> > option in order to disable it.
>
> Well, we could have command line options (for each of the RTC/CMOS,
> PIC, and PIT probing allowing the alias masks to be specified (so we
> don't need to probe). A value of 1 would uniformly mean "no probing,
> no aliases" (as all three decode the low bit, so aliasing can happen
> there). We could further make the default of these variables (yes/no,
> no actual mask values of course) controllable by a Kconfig setting.
If you want to make this more fine grained, or even allow the user to
provide custom masks that's all fine, but there's already
dom0_ioports_disable that allows disabling a list of IO port ranges.
What I would require is a way to avoid all the probing, so that we
could return to the previous behavior.
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -425,6 +425,69 @@ static struct platform_timesource __init
> >> .resume = resume_pit,
> >> };
> >>
> >> +unsigned int __initdata pit_alias_mask;
> >> +
> >> +static void __init probe_pit_alias(void)
> >> +{
> >> + unsigned int mask = 0x1c;
> >> + uint8_t val = 0;
> >> +
> >> + /*
> >> + * Use channel 2 in mode 0 for probing. In this mode even a
> >> non-initial
> >> + * count is loaded independent of counting being / becoming enabled.
> >> Thus
> >> + * we have a 16-bit value fully under our control, to write and then
> >> check
> >> + * whether we can also read it back unaltered.
> >> + */
> >> +
> >> + /* Turn off speaker output and disable channel 2 counting. */
> >> + outb(inb(0x61) & 0x0c, 0x61);
> >> +
> >> + outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */
> >> +
> >> + do {
> >> + uint8_t val2;
> >> + unsigned int offs;
> >> +
> >> + outb(val, PIT_CH2);
> >> + outb(val ^ 0xff, PIT_CH2);
> >> +
> >> + /* Wait for the Null Count bit to clear. */
> >> + do {
> >> + /* Latch status. */
> >> + outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);
> >> +
> >> + /* Try to make sure we're actually having a PIT here. */
> >> + val2 = inb(PIT_CH2);
> >> + if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
> >> + return;
> >> + } while ( val2 & (1 << 6) );
> >
> > We should have some kind of timeout here, just in case...
>
> Hmm, I indeed did consider the need for a timeout here. With what
> we've done up to here we already assume a functioning PIT, verifying
> simply as we go. The issue with truly using some form of timeout is
> the determination of how long to wait at most.
I would likely make it based on iterations, could you get some figures
on how many iterations it takes for the bit to be clear?
I would think something like 1000 should be enough, but really have no
idea.
> >> +
> >> + /*
> >> + * Try to further make sure we're actually having a PIT here.
> >> + *
> >> + * NB: Deliberately |, not ||, as we always want both reads.
> >> + */
> >> + val2 = inb(PIT_CH2);
> >> + if ( (val2 ^ val) | (inb(PIT_CH2) ^ val ^ 0xff) )
> >> + return;
> >> +
> >> + for ( offs = mask & -mask; offs <= mask; offs <<= 1 )
> >> + {
> >> + if ( !(mask & offs) )
> >> + continue;
> >> + val2 = inb(PIT_CH2 + offs);
> >> + if ( (val2 ^ val) | (inb(PIT_CH2 + offs) ^ val ^ 0xff) )
> >> + mask &= ~offs;
> >> + }
> >> + } while ( mask && (val += 0x0b) ); /* Arbitrary uneven number. */
> >> +
> >> + if ( mask )
> >> + {
> >> + dprintk(XENLOG_INFO, "PIT aliasing mask: %02x\n", mask);
> >> + pit_alias_mask = mask;
> >> + }
> >
> > Is it fine to leave counting disabled for channel 2?
> >
> > Shouldn't we restore the previous gate value?
>
> See init_pit(), which also doesn't restore anything. The system is under
> our control, and we have no other use for channel 2. (I really had
> restore logic here initially, but then dropped it for said reason.)
It might be used by a PV dom0 (see hwdom_pit_access()), so I wonder
whether we should try to hand off as Xen found it.
Thanks, Roger.
