diff mbox series

xen/memory: Reject out-of-range resource 'frame' values

Message ID 20210128160616.17608-1-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series xen/memory: Reject out-of-range resource 'frame' values | expand

Commit Message

Andrew Cooper Jan. 28, 2021, 4:06 p.m. UTC
The ABI is unfortunate, and frame being 64 bits leads to all kinds of problems
performing correct overflow checks.

Furthermore, the mixed use of unsigned int and unsigned long in the call tree
is buggy on arm32 where the two are the same size, and certain out-of-range
combinations will truncated and be permitted.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Paul Durrant <paul@xen.org>
CC: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Posted ahead of my full v8 series, in the hope that it catches some people
before the end of the day.
---
 xen/arch/x86/mm.c        |  2 +-
 xen/common/memory.c      | 15 ++++++++++++++-
 xen/include/asm-x86/mm.h |  2 +-
 xen/include/xen/mm.h     |  2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

Comments

Paul Durrant Jan. 28, 2021, 5:13 p.m. UTC | #1
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 28 January 2021 16:06
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Julien
> Grall <julien@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Paul Durrant <paul@xen.org>;
> Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Subject: [PATCH] xen/memory: Reject out-of-range resource 'frame' values
> 
> The ABI is unfortunate, and frame being 64 bits leads to all kinds of problems
> performing correct overflow checks.
> 
> Furthermore, the mixed use of unsigned int and unsigned long in the call tree
> is buggy on arm32 where the two are the same size, and certain out-of-range
> combinations will truncated and be permitted.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Paul Durrant <paul@xen.org>
> CC: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Posted ahead of my full v8 series, in the hope that it catches some people
> before the end of the day.

Reviewed-by: Paul Durrant <paul@xen.org>
Jan Beulich Jan. 29, 2021, 9:15 a.m. UTC | #2
On 28.01.2021 17:06, Andrew Cooper wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1054,7 +1054,7 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>  }
>  
>  static int acquire_grant_table(struct domain *d, unsigned int id,
> -                               unsigned long frame,
> +                               unsigned int frame,
>                                 unsigned int nr_frames,
>                                 xen_pfn_t mfn_list[])
>  {

Doesn't this want carrying forward into
gnttab_get_{shared,status}_frame() as well? Of course further
cleanup here can also be done at a later point, but it leaves
things in a somewhat inconsistent state. I'd like to leave it
up to you to commit with Paul's R-b as is, or extend the
adjustments and then also add mine.

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -641,7 +641,7 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
>  }
>  
>  int arch_acquire_resource(struct domain *d, unsigned int type,
> -                          unsigned int id, unsigned long frame,
> +                          unsigned int id, unsigned int frame,
>                            unsigned int nr_frames, xen_pfn_t mfn_list[]);

Same here wrt hvm_get_ioreq_server_frame().

Jan
Andrew Cooper Jan. 29, 2021, 9:32 a.m. UTC | #3
On 29/01/2021 09:15, Jan Beulich wrote:
> On 28.01.2021 17:06, Andrew Cooper wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1054,7 +1054,7 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>>  }
>>  
>>  static int acquire_grant_table(struct domain *d, unsigned int id,
>> -                               unsigned long frame,
>> +                               unsigned int frame,
>>                                 unsigned int nr_frames,
>>                                 xen_pfn_t mfn_list[])
>>  {
> Doesn't this want carrying forward into
> gnttab_get_{shared,status}_frame() as well? Of course further
> cleanup here can also be done at a later point, but it leaves
> things in a somewhat inconsistent state. I'd like to leave it
> up to you to commit with Paul's R-b as is, or extend the
> adjustments and then also add mine.

In the series, those functions are deleted by the next patch.

What's the likelihood that you'll choose to backport this?  I can extend
it if needs be.

>
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -641,7 +641,7 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
>>  }
>>  
>>  int arch_acquire_resource(struct domain *d, unsigned int type,
>> -                          unsigned int id, unsigned long frame,
>> +                          unsigned int id, unsigned int frame,
>>                            unsigned int nr_frames, xen_pfn_t mfn_list[]);
> Same here wrt hvm_get_ioreq_server_frame().

This one isn't.  I'll adjust.

~Andrew
Jan Beulich Jan. 29, 2021, 9:40 a.m. UTC | #4
On 29.01.2021 10:32, Andrew Cooper wrote:
> On 29/01/2021 09:15, Jan Beulich wrote:
>> On 28.01.2021 17:06, Andrew Cooper wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1054,7 +1054,7 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>>>  }
>>>  
>>>  static int acquire_grant_table(struct domain *d, unsigned int id,
>>> -                               unsigned long frame,
>>> +                               unsigned int frame,
>>>                                 unsigned int nr_frames,
>>>                                 xen_pfn_t mfn_list[])
>>>  {
>> Doesn't this want carrying forward into
>> gnttab_get_{shared,status}_frame() as well? Of course further
>> cleanup here can also be done at a later point, but it leaves
>> things in a somewhat inconsistent state. I'd like to leave it
>> up to you to commit with Paul's R-b as is, or extend the
>> adjustments and then also add mine.
> 
> In the series, those functions are deleted by the next patch.

In your submission you talk about a v8 series, which I took to
mean the vmtrace one. I understand here you refer to the other
series, presently at v3?

> What's the likelihood that you'll choose to backport this?

Didn't consider this aspect yet. I think I wouldn't have picked
it without anyone asking for it to be backported.

>  I can extend it if needs be.

Well, if that deletion of code gets committed in time, then of
course there's no real need to fiddle with it here.

Jan
Andrew Cooper Jan. 29, 2021, 9:47 a.m. UTC | #5
On 29/01/2021 09:40, Jan Beulich wrote:
> On 29.01.2021 10:32, Andrew Cooper wrote:
>> On 29/01/2021 09:15, Jan Beulich wrote:
>>> On 28.01.2021 17:06, Andrew Cooper wrote:
>>>> --- a/xen/common/memory.c
>>>> +++ b/xen/common/memory.c
>>>> @@ -1054,7 +1054,7 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>>>>  }
>>>>  
>>>>  static int acquire_grant_table(struct domain *d, unsigned int id,
>>>> -                               unsigned long frame,
>>>> +                               unsigned int frame,
>>>>                                 unsigned int nr_frames,
>>>>                                 xen_pfn_t mfn_list[])
>>>>  {
>>> Doesn't this want carrying forward into
>>> gnttab_get_{shared,status}_frame() as well? Of course further
>>> cleanup here can also be done at a later point, but it leaves
>>> things in a somewhat inconsistent state. I'd like to leave it
>>> up to you to commit with Paul's R-b as is, or extend the
>>> adjustments and then also add mine.
>> In the series, those functions are deleted by the next patch.
> In your submission you talk about a v8 series, which I took to
> mean the vmtrace one. I understand here you refer to the other
> series, presently at v3?

The two combined is v8 because of the dependencies (and that's what I'll
post all together), but yes - I did mean the thing presently at v3 as
posted.

>
>> What's the likelihood that you'll choose to backport this?
> Didn't consider this aspect yet. I think I wouldn't have picked
> it without anyone asking for it to be backported.
>
>>   I can extend it if needs be.
> Well, if that deletion of code gets committed in time, then of
> course there's no real need to fiddle with it here.

This specific patch fixes a real bug on arm32 which will cause unsigned
long + unsigned int to truncate together and permit certain values.

If you want to backport it, I should extend the change all the way down
the call tree.

The rest of the cleanup really depends on the libxenforeignmem change,
and ioctl fixes in the kernel, and probably aren't suitable for backport.

~Andrew
Jan Beulich Jan. 29, 2021, 10:01 a.m. UTC | #6
On 29.01.2021 10:47, Andrew Cooper wrote:
> On 29/01/2021 09:40, Jan Beulich wrote:
>> On 29.01.2021 10:32, Andrew Cooper wrote:
>>> What's the likelihood that you'll choose to backport this?
>> Didn't consider this aspect yet. I think I wouldn't have picked
>> it without anyone asking for it to be backported.
>>
>>>   I can extend it if needs be.
>> Well, if that deletion of code gets committed in time, then of
>> course there's no real need to fiddle with it here.
> 
> This specific patch fixes a real bug on arm32 which will cause unsigned
> long + unsigned int to truncate together and permit certain values.

Why Arm32 only? Looking at current staging, there's no overflow
check at all on the grant part of the path. A suitably large
64-bit "frame" will allow the same behavior on 64-bit (wrapping
around through zero), afaict.

Jan
Andrew Cooper Jan. 29, 2021, 10:04 a.m. UTC | #7
On 29/01/2021 10:01, Jan Beulich wrote:
> On 29.01.2021 10:47, Andrew Cooper wrote:
>> On 29/01/2021 09:40, Jan Beulich wrote:
>>> On 29.01.2021 10:32, Andrew Cooper wrote:
>>>> What's the likelihood that you'll choose to backport this?
>>> Didn't consider this aspect yet. I think I wouldn't have picked
>>> it without anyone asking for it to be backported.
>>>
>>>>   I can extend it if needs be.
>>> Well, if that deletion of code gets committed in time, then of
>>> course there's no real need to fiddle with it here.
>> This specific patch fixes a real bug on arm32 which will cause unsigned
>> long + unsigned int to truncate together and permit certain values.
> Why Arm32 only? Looking at current staging, there's no overflow
> check at all on the grant part of the path. A suitably large
> 64-bit "frame" will allow the same behavior on 64-bit (wrapping
> around through zero), afaict.

Very good point.  I'd worked the logic through logically at the end of
my fixes, rather than at its position in the beginning of the series.

In which case I'll propagate through the whole call-tree.

~Andrew
Jan Beulich Jan. 29, 2021, 10:09 a.m. UTC | #8
On 29.01.2021 11:04, Andrew Cooper wrote:
> On 29/01/2021 10:01, Jan Beulich wrote:
>> On 29.01.2021 10:47, Andrew Cooper wrote:
>>> On 29/01/2021 09:40, Jan Beulich wrote:
>>>> On 29.01.2021 10:32, Andrew Cooper wrote:
>>>>> What's the likelihood that you'll choose to backport this?
>>>> Didn't consider this aspect yet. I think I wouldn't have picked
>>>> it without anyone asking for it to be backported.
>>>>
>>>>>   I can extend it if needs be.
>>>> Well, if that deletion of code gets committed in time, then of
>>>> course there's no real need to fiddle with it here.
>>> This specific patch fixes a real bug on arm32 which will cause unsigned
>>> long + unsigned int to truncate together and permit certain values.
>> Why Arm32 only? Looking at current staging, there's no overflow
>> check at all on the grant part of the path. A suitably large
>> 64-bit "frame" will allow the same behavior on 64-bit (wrapping
>> around through zero), afaict.
> 
> Very good point.  I'd worked the logic through logically at the end of
> my fixes, rather than at its position in the beginning of the series.
> 
> In which case I'll propagate through the whole call-tree.

Thanks. And then, as said,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

And I guess I'll try to remember to pick this up for backporting
once it has landed.

Jan
Julien Grall Jan. 29, 2021, 11:43 a.m. UTC | #9
Hi Andrew,

On 28/01/2021 16:06, Andrew Cooper wrote:
> The ABI is unfortunate, and frame being 64 bits leads to all kinds of problems
> performing correct overflow checks.
> 
> Furthermore, the mixed use of unsigned int and unsigned long in the call tree
> is buggy on arm32 where the two are the same size, and certain out-of-range
> combinations will truncated and be permitted.

set_foreign_p2m_entry() always return -EOPNOTSUPP, so I think this would 
not need a backport if the issue is Arm only.

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

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 63e9fae919..2ce00a8ece 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4588,7 +4588,7 @@  static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
 }
 
 int arch_acquire_resource(struct domain *d, unsigned int type,
-                          unsigned int id, unsigned long frame,
+                          unsigned int id, unsigned int frame,
                           unsigned int nr_frames, xen_pfn_t mfn_list[])
 {
     int rc;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index ccb4d49fc6..33ab36eb72 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1054,7 +1054,7 @@  static long xatp_permission_check(struct domain *d, unsigned int space)
 }
 
 static int acquire_grant_table(struct domain *d, unsigned int id,
-                               unsigned long frame,
+                               unsigned int frame,
                                unsigned int nr_frames,
                                xen_pfn_t mfn_list[])
 {
@@ -1134,6 +1134,19 @@  static int acquire_resource(
     if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
         return -E2BIG;
 
+    /*
+     * The ABI is rather unfortunate.  nr_frames (and therefore the total size
+     * of the resource) is 32bit, while frame (the offset within the resource
+     * we'd like to start at) is 64bit.
+     *
+     * Reject values oustide the of the range of nr_frames, as well as
+     * combinations of frame and nr_frame which overflow, to simplify the rest
+     * of the logic.
+     */
+    if ( (xmar.frame >> 32) ||
+         ((xmar.frame + xmar.nr_frames) >> 32) )
+        return -EINVAL;
+
     rc = rcu_lock_remote_domain_by_id(xmar.domid, &d);
     if ( rc )
         return rc;
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 1fdb4eb835..3fe47dcd8d 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -641,7 +641,7 @@  static inline bool arch_mfn_in_directmap(unsigned long mfn)
 }
 
 int arch_acquire_resource(struct domain *d, unsigned int type,
-                          unsigned int id, unsigned long frame,
+                          unsigned int id, unsigned int frame,
                           unsigned int nr_frames, xen_pfn_t mfn_list[]);
 
 #endif /* __ASM_X86_MM_H__ */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 636a1254ae..66a4d10695 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -614,7 +614,7 @@  static inline void put_page_alloc_ref(struct page_info *page)
 
 #ifndef CONFIG_ARCH_ACQUIRE_RESOURCE
 static inline int arch_acquire_resource(
-    struct domain *d, unsigned int type, unsigned int id, unsigned long frame,
+    struct domain *d, unsigned int type, unsigned int id, unsigned int frame,
     unsigned int nr_frames, xen_pfn_t mfn_list[])
 {
     return -EOPNOTSUPP;