Re: [PATCH] x86: guard against port I/O overlapping the RTC/CMOS range

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

On Mon, Jul 20, 2020 at 01:58:40PM +0200, Jan Beulich wrote:
> On 20.07.2020 12:52, Roger Pau Monné wrote:
> > On Fri, Jul 17, 2020 at 03:10:43PM +0200, Jan Beulich wrote:
> >> Since we intercept RTC/CMOS port accesses, let's do so consistently in
> >> all cases, i.e. also for e.g. a dword access to [006E,0071]. To avoid
> >> the risk of unintended impact on Dom0 code actually doing so (despite
> >> the belief that none ought to exist), also extend
> >> guest_io_{read,write}() to decompose accesses where some ports are
> >> allowed to be directly accessed and some aren't.
> > 
> > Wouldn't the same apply to displaced accesses to port 0xcf8?
> 
> No, CF8 is special - partial accesses have no meaning as to the
> index selection for subsequent CFC accesses. Or else CF9
> couldn't be a standalone port with entirely different
> functionality..

Right:

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

See below.

> >> @@ -373,25 +384,31 @@ static int read_io(unsigned int port, un
> >>      return X86EMUL_OKAY;
> >>  }
> >>  
> >> +static void _guest_io_write(unsigned int port, unsigned int bytes,
> >> +                            uint32_t data)
> > 
> > There's nothing guest specific about this function I think? If so you
> > could drop the _guest_ prefix and just name it io_write?
> 
> Hmm, when choosing the name I decided that (a) it's a helper of
> the other function and (b) it's still guest driven data that we
> output.

Well, the fact that it's guest driven data shouldn't matter much,
because there are no guest-specific checks in the function anyway - it
might as well be used for non-guest driven data AFAICT? (even if it's
not the case ATM).

It's likely that if I have to change code here in the future I will
drop such prefix, but the change is correct regardless of the naming,
so I'm not going to insist.

> >> +{
> >> +    switch ( bytes )
> >> +    {
> >> +    case 1:
> >> +        outb((uint8_t)data, port);
> >> +        if ( amd_acpi_c1e_quirk )
> >> +            amd_check_disable_c1e(port, (uint8_t)data);
> >> +        break;
> >> +    case 2:
> >> +        outw((uint16_t)data, port);
> >> +        break;
> >> +    case 4:
> >> +        outl(data, port);
> >> +        break;
> >> +    }
> > 
> > Newlines after break statements would be nice, and maybe add a
> > default: ASSERT_UNREACHABLE() case to be on the safe side?
> 
> Well, yes, I guess I should. But then if I edit this moved code,
> I guess I'll also get rid of the stray casts.

Was going to also ask for that, but I assumed there might we some
value in making the truncations explicit here. Feel free to drop those
also if you end up making the above adjustments.

Thanks, Roger.