Patchwork [7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses

login
register
mail settings
Submitter Andrew Cooper
Date Sept. 12, 2017, 12:14 p.m.
Message ID <1505218486-4416-8-git-send-email-andrew.cooper3@citrix.com>
Download mbox | patch
Permalink /patch/336595/
State New
Headers show

Comments

Andrew Cooper - Sept. 12, 2017, 12:14 p.m.
The grant ABI uses 64 bit values, and allows a PV guest to specify linear
addresses.  There is nothing interesting a 32bit PV guest can reference which
will pass an __addr_ok() check, but it should still get an error for trying.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/mm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Jan Beulich - Sept. 12, 2017, 3:50 p.m.
>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
> addresses.  There is nothing interesting a 32bit PV guest can reference which
> will pass an __addr_ok() check, but it should still get an error for trying.

While I'm all for tightening checks, I'm not sure we reasonably can:
Existing guests may (perhaps inadvertently) rely on this behavior,
and hence may break with the change. I think a prereq to this is to
have a command line option (or even a per-guest one) to control
strict vs relaxed argument checking behavior, and tie the extra
checks to that option being true.

Jan
Andrew Cooper - Sept. 12, 2017, 4:04 p.m.
On 12/09/17 16:50, Jan Beulich wrote:
>>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
>> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
>> addresses.  There is nothing interesting a 32bit PV guest can reference which
>> will pass an __addr_ok() check, but it should still get an error for trying.
> While I'm all for tightening checks, I'm not sure we reasonably can:
> Existing guests may (perhaps inadvertently) rely on this behavior,
> and hence may break with the change. I think a prereq to this is to
> have a command line option (or even a per-guest one) to control
> strict vs relaxed argument checking behavior, and tie the extra
> checks to that option being true.

At the moment, any attempt to use this behaviour will still cause a
general error, because we cant locate an L1e mapping the out-of-range
linear address.  Therefore, the guest wouldn't have had the grant
operation succeed before.

The problem is that its a latent security bug if we ever chose to reuse
these ranges for other purposes.

E.g. One idea I've had for a while is to move the XLAT translation logic
into guest mode, accessed via a modification to the hypercall page. 
This would mitigate security issues such as infinite loops or boundary
overflows, both of which we've had in the XLAT logic in the past.

~Andrew
Jan Beulich - Sept. 13, 2017, 8:28 a.m.
>>> On 12.09.17 at 18:04, <andrew.cooper3@citrix.com> wrote:
> On 12/09/17 16:50, Jan Beulich wrote:
>>>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
>>> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
>>> addresses.  There is nothing interesting a 32bit PV guest can reference which
>>> will pass an __addr_ok() check, but it should still get an error for trying.
>> While I'm all for tightening checks, I'm not sure we reasonably can:
>> Existing guests may (perhaps inadvertently) rely on this behavior,
>> and hence may break with the change. I think a prereq to this is to
>> have a command line option (or even a per-guest one) to control
>> strict vs relaxed argument checking behavior, and tie the extra
>> checks to that option being true.
> 
> At the moment, any attempt to use this behaviour will still cause a
> general error, because we cant locate an L1e mapping the out-of-range
> linear address.  Therefore, the guest wouldn't have had the grant
> operation succeed before.

Oh, good point.

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

Jan

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1a8ad42..edf8fdf 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3864,6 +3864,10 @@  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
     }
     else
     {
+        /* Guest trying to pass an out-of-range linear address? */
+        if ( is_pv_32bit_domain(currd) && addr != (uint32_t)addr )
+            goto out;
+
         pl1e = map_guest_l1e(addr, &gl1mfn);
 
         if ( !pl1e )
@@ -4008,6 +4012,19 @@  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     }
     else
     {
+        if ( is_pv_32bit_domain(currd) )
+        {
+            if ( addr != (uint32_t)addr )
+            {
+                ASSERT_UNREACHABLE();
+                goto out;
+            }
+
+            /* Guest trying to pass an out-of-range linear address? */
+            if ( new_addr != (uint32_t)new_addr )
+                goto out;
+        }
+
         if ( new_addr && !steal_linear_address(new_addr, &nl1e) )
             goto out;