Re: [PATCH 5/7] x86: detect PIT aliasing on ports other than 0x4[0-3]

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Oct 26, 2023 at 05:10:41PM +0200, Jan Beulich wrote:
> On 26.10.2023 15:57, Roger Pau Monné wrote:
> > 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.
> 
> Except that how long a given number of iterations takes is unknown. 1000
> may be enough today or on the systems we test, but may not be tomorrow
> or on other peoples' systems. Hence why I'm hesitant ...

Hm, but getting stuck in a loop here can't be good either.  Let's do
it time wise if you prefer, 1s should be more than enough I would
think.

In any case the worse that could if the timeout is hit is that aliases
are not properly detected, but that's way better than the possibility
of getting stuck in an infinite loop.

Thanks, Roger.