| Message ID | 20220825114004.24843-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op() | expand |
On 25.08.2022 13:40, Juergen Gross wrote: > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -581,7 +581,7 @@ static int lock_pages( > struct privcmd_dm_op_buf kbufs[], unsigned int num, > struct page *pages[], unsigned int nr_pages, unsigned int *pinned) > { > - unsigned int i; > + unsigned int i, off = 0; > > for (i = 0; i < num; i++) { > unsigned int requested; > @@ -589,19 +589,23 @@ static int lock_pages( > > requested = DIV_ROUND_UP( > offset_in_page(kbufs[i].uptr) + kbufs[i].size, > - PAGE_SIZE); > + PAGE_SIZE) - off; > if (requested > nr_pages) > return -ENOSPC; > > page_count = pin_user_pages_fast( > - (unsigned long) kbufs[i].uptr, > + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE, > requested, FOLL_WRITE, pages); > - if (page_count < 0) > - return page_count; > + if (page_count <= 0) > + return page_count ? : -EFAULT; > > *pinned += page_count; > nr_pages -= page_count; > pages += page_count; > + > + off = requested - page_count; > + if (off) > + i--; > } Initially I thought this would go wrong only on the 3rd iteration, but meanwhile I think it's wrong already on the 2nd. What I think you need is if (page_count < requested) i--; off += page_count; or with the i++ from the loop header absorbed here if (page_count == requested) i++; off += page_count; Plus of course off needs resetting to zero whenever i advances. I.e. if (page_count == requested) { i++; off = 0; } else { off += page_count; } Jan
On 25.08.22 13:58, Jan Beulich wrote: > On 25.08.2022 13:40, Juergen Gross wrote: >> --- a/drivers/xen/privcmd.c >> +++ b/drivers/xen/privcmd.c >> @@ -581,7 +581,7 @@ static int lock_pages( >> struct privcmd_dm_op_buf kbufs[], unsigned int num, >> struct page *pages[], unsigned int nr_pages, unsigned int *pinned) >> { >> - unsigned int i; >> + unsigned int i, off = 0; >> >> for (i = 0; i < num; i++) { >> unsigned int requested; >> @@ -589,19 +589,23 @@ static int lock_pages( >> >> requested = DIV_ROUND_UP( >> offset_in_page(kbufs[i].uptr) + kbufs[i].size, >> - PAGE_SIZE); >> + PAGE_SIZE) - off; >> if (requested > nr_pages) >> return -ENOSPC; >> >> page_count = pin_user_pages_fast( >> - (unsigned long) kbufs[i].uptr, >> + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE, >> requested, FOLL_WRITE, pages); >> - if (page_count < 0) >> - return page_count; >> + if (page_count <= 0) >> + return page_count ? : -EFAULT; >> >> *pinned += page_count; >> nr_pages -= page_count; >> pages += page_count; >> + >> + off = requested - page_count; >> + if (off) >> + i--; >> } > > Initially I thought this would go wrong only on the 3rd iteration, but > meanwhile I think it's wrong already on the 2nd. What I think you need > is > > if (page_count < requested) > i--; > off += page_count; > > or with the i++ from the loop header absorbed here > > if (page_count == requested) > i++; > off += page_count; > > Plus of course off needs resetting to zero whenever i advances. I.e. > > if (page_count == requested) { > i++; > off = 0; > } else { > off += page_count; > } Yeah, or: off = (page_count == requested) ? 0 : off + page_count; i += !off; Juergen
On 25.08.2022 14:10, Juergen Gross wrote: > On 25.08.22 13:58, Jan Beulich wrote: >> On 25.08.2022 13:40, Juergen Gross wrote: >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -581,7 +581,7 @@ static int lock_pages( >>> struct privcmd_dm_op_buf kbufs[], unsigned int num, >>> struct page *pages[], unsigned int nr_pages, unsigned int *pinned) >>> { >>> - unsigned int i; >>> + unsigned int i, off = 0; >>> >>> for (i = 0; i < num; i++) { >>> unsigned int requested; >>> @@ -589,19 +589,23 @@ static int lock_pages( >>> >>> requested = DIV_ROUND_UP( >>> offset_in_page(kbufs[i].uptr) + kbufs[i].size, >>> - PAGE_SIZE); >>> + PAGE_SIZE) - off; >>> if (requested > nr_pages) >>> return -ENOSPC; >>> >>> page_count = pin_user_pages_fast( >>> - (unsigned long) kbufs[i].uptr, >>> + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE, >>> requested, FOLL_WRITE, pages); >>> - if (page_count < 0) >>> - return page_count; >>> + if (page_count <= 0) >>> + return page_count ? : -EFAULT; >>> >>> *pinned += page_count; >>> nr_pages -= page_count; >>> pages += page_count; >>> + >>> + off = requested - page_count; >>> + if (off) >>> + i--; >>> } >> >> Initially I thought this would go wrong only on the 3rd iteration, but >> meanwhile I think it's wrong already on the 2nd. What I think you need >> is >> >> if (page_count < requested) >> i--; >> off += page_count; >> >> or with the i++ from the loop header absorbed here >> >> if (page_count == requested) >> i++; >> off += page_count; >> >> Plus of course off needs resetting to zero whenever i advances. I.e. >> >> if (page_count == requested) { >> i++; >> off = 0; >> } else { >> off += page_count; >> } > > Yeah, or: > > off = (page_count == requested) ? 0 : off + page_count; > i += !off; I wasn't daring to suggest something like that ;-) Jan
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 3369734108af..1ca7e3ea6fd4 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -581,7 +581,7 @@ static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, struct page *pages[], unsigned int nr_pages, unsigned int *pinned) { - unsigned int i; + unsigned int i, off = 0; for (i = 0; i < num; i++) { unsigned int requested; @@ -589,19 +589,23 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, - PAGE_SIZE); + PAGE_SIZE) - off; if (requested > nr_pages) return -ENOSPC; page_count = pin_user_pages_fast( - (unsigned long) kbufs[i].uptr, + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE, requested, FOLL_WRITE, pages); - if (page_count < 0) - return page_count; + if (page_count <= 0) + return page_count ? : -EFAULT; *pinned += page_count; nr_pages -= page_count; pages += page_count; + + off = requested - page_count; + if (off) + i--; } return 0; @@ -677,10 +681,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) } rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned); - if (rc < 0) { - nr_pages = pinned; + if (rc < 0) goto out; - } for (i = 0; i < kdata.num; i++) { set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr); @@ -692,7 +694,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + unlock_pages(pages, pinned); kfree(xbufs); kfree(pages); kfree(kbufs);
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages() potentially with pages being NULL, leading to a NULL dereference. Additionally lock_pages() doesn't check for pin_user_pages_fast() having been completely successful, resulting in potentially not locking all pages into memory. This could result in sporadic failures when using the related memory in user mode. Fix all of that by calling unlock_pages() always with the real number of pinned pages, which will be zero in case pages being NULL, and by checking the number of pages pinned by pin_user_pages_fast() matching the expected number of pages. Cc: <stable@vger.kernel.org> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP") Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru> Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - use "pinned" as parameter for unlock_pages() (Jan Beulich) - drop label "unlock" again (Jan Beulich) - add check for complete success of pin_user_pages_fast() V3: - continue after partial success of pin_user_pages_fast() (Jan Beulich) --- drivers/xen/privcmd.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)