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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.