diff mbox

x86/mmuext: don't allow copying/clearing non-RAM pages

Message ID 594A6241020000780016517B@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich June 21, 2017, 10:10 a.m. UTC 
The two operations really aren't meant for anything else.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86/mmuext: don't allow copying/clearing non-RAM pages

The two operations really aren't meant for anything else.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3229,6 +3229,7 @@ long do_mmuext_op(
         switch ( op.cmd )
         {
             struct page_info *page;
+            p2m_type_t p2mt;
 
         case MMUEXT_PIN_L1_TABLE:
             type = PGT_l1_page_table;
@@ -3528,7 +3529,12 @@ long do_mmuext_op(
         }
 
         case MMUEXT_CLEAR_PAGE:
-            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt, P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && page )
+            {
+                put_page(page);
+                page = NULL;
+            }
             if ( !page || !get_page_type(page, PGT_writable_page) )
             {
                 if ( page )
@@ -3551,8 +3557,13 @@ long do_mmuext_op(
         {
             struct page_info *src_page, *dst_page;
 
-            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, NULL,
+            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, &p2mt,
                                          P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && src_page )
+            {
+                put_page(src_page);
+                src_page = NULL;
+            }
             if ( unlikely(!src_page) )
             {
                 gdprintk(XENLOG_WARNING,
@@ -3562,8 +3573,13 @@ long do_mmuext_op(
                 break;
             }
 
-            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
+            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt,
                                          P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && dst_page )
+            {
+                put_page(dst_page);
+                dst_page = NULL;
+            }
             rc = (dst_page &&
                   get_page_type(dst_page, PGT_writable_page)) ? 0 : -EINVAL;
             if ( unlikely(rc) )

Comments

Andrew Cooper June 21, 2017, 10:51 a.m. UTC | #1 
On 21/06/17 11:10, Jan Beulich wrote:
> The two operations really aren't meant for anything else.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, however...

>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3229,6 +3229,7 @@ long do_mmuext_op(
>          switch ( op.cmd )
>          {
>              struct page_info *page;
> +            p2m_type_t p2mt;
>  
>          case MMUEXT_PIN_L1_TABLE:
>              type = PGT_l1_page_table;
> @@ -3528,7 +3529,12 @@ long do_mmuext_op(
>          }
>  
>          case MMUEXT_CLEAR_PAGE:
> -            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
> +            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt, P2M_ALLOC);
> +            if ( unlikely(p2mt != p2m_ram_rw) && page )

... it would seem more natural to have the null pointer check before the
p2mt check.

~Andrew
Jan Beulich June 21, 2017, 11:47 a.m. UTC | #2 
>>> On 21.06.17 at 12:51, <andrew.cooper3@citrix.com> wrote:
> On 21/06/17 11:10, Jan Beulich wrote:
>> The two operations really aren't meant for anything else.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, however...
> 
>>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -3229,6 +3229,7 @@ long do_mmuext_op(
>>          switch ( op.cmd )
>>          {
>>              struct page_info *page;
>> +            p2m_type_t p2mt;
>>  
>>          case MMUEXT_PIN_L1_TABLE:
>>              type = PGT_l1_page_table;
>> @@ -3528,7 +3529,12 @@ long do_mmuext_op(
>>          }
>>  
>>          case MMUEXT_CLEAR_PAGE:
>> -            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
>> +            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt, P2M_ALLOC);
>> +            if ( unlikely(p2mt != p2m_ram_rw) && page )
> 
> ... it would seem more natural to have the null pointer check before the
> p2mt check.

Since the checks are independent, the order doesn't really matter,
and with that it seemed better to put the unlikely() first (to get the
other check off the fast path).

Jan
diff mbox

Patch

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3229,6 +3229,7 @@  long do_mmuext_op(
         switch ( op.cmd )
         {
             struct page_info *page;
+            p2m_type_t p2mt;
 
         case MMUEXT_PIN_L1_TABLE:
             type = PGT_l1_page_table;
@@ -3528,7 +3529,12 @@  long do_mmuext_op(
         }
 
         case MMUEXT_CLEAR_PAGE:
-            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt, P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && page )
+            {
+                put_page(page);
+                page = NULL;
+            }
             if ( !page || !get_page_type(page, PGT_writable_page) )
             {
                 if ( page )
@@ -3551,8 +3557,13 @@  long do_mmuext_op(
         {
             struct page_info *src_page, *dst_page;
 
-            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, NULL,
+            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, &p2mt,
                                          P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && src_page )
+            {
+                put_page(src_page);
+                src_page = NULL;
+            }
             if ( unlikely(!src_page) )
             {
                 gdprintk(XENLOG_WARNING,
@@ -3562,8 +3573,13 @@  long do_mmuext_op(
                 break;
             }
 
-            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
+            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt,
                                          P2M_ALLOC);
+            if ( unlikely(p2mt != p2m_ram_rw) && dst_page )
+            {
+                put_page(dst_page);
+                dst_page = NULL;
+            }
             rc = (dst_page &&
                   get_page_type(dst_page, PGT_writable_page)) ? 0 : -EINVAL;
             if ( unlikely(rc) )