[PATCH 0/3] XENMEM_add_to_physmap handling adjustments

[PATCH 0/3] XENMEM_add_to_physmap handling adjustments

[Jan Beulich]

I'd like to put up the following three changes for consideration of
inclusion in 4.10. Especially when taking only release builds into
account (relevant to patch 1), they're not much more than
cosmetic changes, but imo they still are an overall improvement to
code quality.

1: x86: replace bad ASSERT() in xenmem_add_to_physmap_one()
2: x86: check paging mode earlier in xenmem_add_to_physmap_one()
3: improve XENMEM_add_to_physmap_batch address checking

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] x86: replace bad ASSERT() in xenmem_add_to_physmap_one()

[Jan Beulich]

There are no locks being held, i.e. it is possible to be triggered by
racy hypercall invocations. Subsequent code doesn't really depend on the
checked values, so this is not a security issue.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm up for to better suggestions for the EXDEV I've used; I certainly
don't want to use -EINVAL, and -EAGAIN (afaik) generally is meant as a
hint to re-invoke with the same arguments (which wouldn't help here).

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4143,8 +4143,12 @@ int xenmem_add_to_physmap_one(
     /* Unmap from old location, if any. */
     old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
     ASSERT( old_gpfn != SHARED_M2P_ENTRY );
-    if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
-        ASSERT( old_gpfn == gfn );
+    if ( (space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range) &&
+         old_gpfn != gfn )
+    {
+        rc = -EXDEV;
+        goto put_both;
+    }
     if ( old_gpfn != INVALID_M2P_ENTRY )
         rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] x86: check paging mode earlier in xenmem_add_to_physmap_one()

[Jan Beulich]

There's no point in deferring this until after some initial processing,
and it's actively wrong for the XENMAPSPACE_gmfn_foreign handling to not
have such a check at all.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4081,6 +4081,9 @@ int xenmem_add_to_physmap_one(
     mfn_t mfn = INVALID_MFN;
     p2m_type_t p2mt;
 
+    if ( !paging_mode_translate(d) )
+        return -EACCES;
+
     switch ( space )
     {
         case XENMAPSPACE_shared_info:
@@ -4117,7 +4120,7 @@ int xenmem_add_to_physmap_one(
             break;
     }
 
-    if ( !paging_mode_translate(d) || mfn_eq(mfn, INVALID_MFN) )
+    if ( mfn_eq(mfn, INVALID_MFN) )
     {
         rc = -EINVAL;
         goto put_both;




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] improve XENMEM_add_to_physmap_batch address checking

[Jan Beulich]

As a follow-up to XSA-212 we should have addressed a similar issue here:
The handles being advanced at the top of xenmem_add_to_physmap_batch()
means we allow hypervisor space accesses (in particular, for "errs",
writes) with suitably crafted input arguments. This isn't a security
issue in this case because of the limited width of struct
xen_add_to_physmap_batch's size field: It being 16-bits wide, only the
r/o M2P area can be accessed. Still we can and should do better.

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

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -823,6 +823,11 @@ static int xenmem_add_to_physmap_batch(s
     guest_handle_add_offset(xatpb->errs, start);
     xatpb->size -= start;
 
+    if ( !guest_handle_okay(xatpb->idxs, xatpb->size) ||
+         !guest_handle_okay(xatpb->gpfns, xatpb->size) ||
+         !guest_handle_okay(xatpb->errs, xatpb->size) )
+        return -EFAULT;
+
     while ( xatpb->size > done )
     {
         xen_ulong_t idx;
@@ -1141,10 +1146,7 @@ long do_memory_op(unsigned long cmd, XEN
         if ( start_extent != (typeof(xatpb.size))start_extent )
             return -EDOM;
 
-        if ( copy_from_guest(&xatpb, arg, 1) ||
-             !guest_handle_okay(xatpb.idxs, xatpb.size) ||
-             !guest_handle_okay(xatpb.gpfns, xatpb.size) ||
-             !guest_handle_okay(xatpb.errs, xatpb.size) )
+        if ( copy_from_guest(&xatpb, arg, 1) )
             return -EFAULT;
 
         /* This mapspace is unsupported for this hypercall. */




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86: check paging mode earlier in xenmem_add_to_physmap_one()

[George Dunlap]

On 11/27/2017 09:12 AM, Jan Beulich wrote:
> There's no point in deferring this until after some initial processing,
> and it's actively wrong for the XENMAPSPACE_gmfn_foreign handling to not
> have such a check at all.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

I also agree that this should be considered for inclusion for 4.10.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] x86: replace bad ASSERT() in xenmem_add_to_physmap_one()

[Andrew Cooper]

On 27/11/17 09:11, Jan Beulich wrote:
> There are no locks being held, i.e. it is possible to be triggered by
> racy hypercall invocations. Subsequent code doesn't really depend on the
> checked values, so this is not a security issue.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> I'm up for to better suggestions for the EXDEV I've used; I certainly
> don't want to use -EINVAL, and -EAGAIN (afaik) generally is meant as a
> hint to re-invoke with the same arguments (which wouldn't help here).

I can't offer a better suggestion.

>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4143,8 +4143,12 @@ int xenmem_add_to_physmap_one(
>      /* Unmap from old location, if any. */
>      old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
>      ASSERT( old_gpfn != SHARED_M2P_ENTRY );
> -    if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> -        ASSERT( old_gpfn == gfn );
> +    if ( (space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range) &&
> +         old_gpfn != gfn )
> +    {
> +        rc = -EXDEV;
> +        goto put_both;
> +    }
>      if ( old_gpfn != INVALID_M2P_ENTRY )
>          rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
>  
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86: check paging mode earlier in xenmem_add_to_physmap_one()

[Andrew Cooper]

On 27/11/17 09:12, Jan Beulich wrote:
> There's no point in deferring this until after some initial processing,
> and it's actively wrong for the XENMAPSPACE_gmfn_foreign handling to not
> have such a check at all.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] improve XENMEM_add_to_physmap_batch address checking

[Andrew Cooper]

On 27/11/17 09:12, Jan Beulich wrote:
> As a follow-up to XSA-212 we should have addressed a similar issue here:
> The handles being advanced at the top of xenmem_add_to_physmap_batch()
> means we allow hypervisor space accesses (in particular, for "errs",
> writes) with suitably crafted input arguments. This isn't a security
> issue in this case because of the limited width of struct
> xen_add_to_physmap_batch's size field: It being 16-bits wide, only the
> r/o M2P area can be accessed. Still we can and should do better.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] XENMEM_add_to_physmap handling adjustments

[Julien Grall]

On 27/11/17 08:34, Jan Beulich wrote:
> I'd like to put up the following three changes for consideration of
> inclusion in 4.10. Especially when taking only release builds into
> account (relevant to patch 1), they're not much more than
> cosmetic changes, but imo they still are an overall improvement to
> code quality.
> 
> 1: x86: replace bad ASSERT() in xenmem_add_to_physmap_one()
> 2: x86: check paging mode earlier in xenmem_add_to_physmap_one()
> 3: improve XENMEM_add_to_physmap_batch address checking
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] x86: replace bad ASSERT() in xenmem_add_to_physmap_one()

[George Dunlap]

On 11/27/2017 09:11 AM, Jan Beulich wrote:
> There are no locks being held, i.e. it is possible to be triggered by
> racy hypercall invocations. Subsequent code doesn't really depend on the
> checked values, so this is not a security issue.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel