| Message ID | 20210128160616.17608-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/memory: Reject out-of-range resource 'frame' values | expand |
> -----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>
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
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
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
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
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
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
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
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 --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;
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(-)