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

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

[Jan Beulich]

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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)
+{
+    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;
+    }
+}
+
 static void guest_io_write(unsigned int port, unsigned int bytes,
                            uint32_t data, struct domain *currd)
 {
     if ( admin_io_okay(port, bytes, currd) )
     {
-        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;
-        }
+        _guest_io_write(port, bytes, data);
         return;
     }
 
@@ -420,6 +437,13 @@ static void guest_io_write(unsigned int
             if ( pci_cfg_ok(currd, port & 3, size, &data) )
                 pci_conf_write(currd->arch.pci_cf8, port & 3, size, data);
         }
+        else if ( ioports_access_permitted(currd, port, port) )
+        {
+            if ( bytes > 1 && !(port & 1) &&
+                 ioports_access_permitted(currd, port, port + 1) )
+                size = 2;
+            _guest_io_write(port, size, data);
+        }
 
         if ( size == 4 )
             return;

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

[Roger Pau Monné]

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@suse.com>
> 
> --- 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.

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

[Jan Beulich]

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..

>> @@ -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.

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

Jan

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

[Roger Pau Monné]

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@citrix.com>

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.

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

[Andrew Cooper]

On 17/07/2020 14:10, 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- 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;

This first hunk is fine.

However, why decompose anything?  Any disallowed port in the range
terminates the entire access, and doesn't internally shrink the access.

~Andrew

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

[Jan Beulich]

On 24.07.2020 14:11, Andrew Cooper wrote:
> On 17/07/2020 14:10, 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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- 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;
> 
> This first hunk is fine.
> 
> However, why decompose anything?  Any disallowed port in the range
> terminates the entire access, and doesn't internally shrink the access.

What tells you that adjacent ports (e.g. 006E and 006F to match
the example in the description) are disallowed? The typical
case here is Dom0 (as mentioned in the description), which has
access to most of the ports.

Jan

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

[Jan Beulich]

On 24.07.2020 16:19, Jan Beulich wrote:
> On 24.07.2020 14:11, Andrew Cooper wrote:
>> On 17/07/2020 14:10, 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.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- 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;
>>
>> This first hunk is fine.
>>
>> However, why decompose anything?  Any disallowed port in the range
>> terminates the entire access, and doesn't internally shrink the access.
> 
> What tells you that adjacent ports (e.g. 006E and 006F to match
> the example in the description) are disallowed? The typical
> case here is Dom0 (as mentioned in the description), which has
> access to most of the ports.

Are you okay with this answer, and hence may I commit the change
with Roger's R-b (and the cosmetic adjustments he did ask for)?
(Unless I hear otherwise within the next day or two, I guess I'll
assume so.)

Jan