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

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

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?

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -210,7 +210,7 @@ static bool admin_io_okay(unsigned int p
>          return false;
>  
>      /* We also never permit direct access to the RTC/CMOS registers. */
> -    if ( ((port & ~1) == RTC_PORT(0)) )
> +    if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
>          return false;
>  
>      return ioports_access_permitted(d, port, port + bytes - 1);
> @@ -297,6 +297,17 @@ static uint32_t guest_io_read(unsigned i
>              if ( pci_cfg_ok(currd, port & 3, size, NULL) )
>                  sub_data = pci_conf_read(currd->arch.pci_cf8, port & 3, 
> size);
>          }
> +        else if ( ioports_access_permitted(currd, port, port) )
> +        {
> +            if ( bytes > 1 && !(port & 1) &&
> +                 ioports_access_permitted(currd, port, port + 1) )
> +            {
> +                sub_data = inw(port);
> +                size = 2;
> +            }
> +            else
> +                sub_data = inb(port);
> +        }
>  
>          if ( size == 4 )
>              return sub_data;
> @@ -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?

> +{
> +    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?

Thanks, Roger.