| Message ID | 20220825092600.7188-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op() | expand |
On 25.08.2022 11:26, Juergen Gross wrote: > 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 patches pinned by pin_user_pages_fast() Nit: s/patches/pages/ > 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> Reviewed-by: Jan Beulich <jbeulich@suse.com> I have a question / suggestion, though: > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -602,6 +602,10 @@ static int lock_pages( > *pinned += page_count; > nr_pages -= page_count; > pages += page_count; > + > + /* Exact reason isn't known, EFAULT is one possibility. */ > + if (page_count < requested) > + return -EFAULT; > } I don't really know the inner workings of pin_user_pages_fast() nor what future plans there are with it. To be as independent of its behavior as possible, how about bailing here only when page_count actually is zero (i.e. no forward progress)? Jan
On 25.08.22 11:50, Jan Beulich wrote: > On 25.08.2022 11:26, Juergen Gross wrote: >> 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 patches pinned by pin_user_pages_fast() > > Nit: s/patches/pages/ > >> 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> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > I have a question / suggestion, though: > >> --- a/drivers/xen/privcmd.c >> +++ b/drivers/xen/privcmd.c >> @@ -602,6 +602,10 @@ static int lock_pages( >> *pinned += page_count; >> nr_pages -= page_count; >> pages += page_count; >> + >> + /* Exact reason isn't known, EFAULT is one possibility. */ >> + if (page_count < requested) >> + return -EFAULT; >> } > > I don't really know the inner workings of pin_user_pages_fast() > nor what future plans there are with it. To be as independent of > its behavior as possible, how about bailing here only when > page_count actually is zero (i.e. no forward progress)? This would require to rework the loop in lock_pages() to be able to handle only a partial buffer. This would add some complexity, but OTOH I'd get an exact error code back in case of failure. I'll have a try and see how the result would look like. Juergen
On 25.08.2022 12:13, Juergen Gross wrote: > On 25.08.22 11:50, Jan Beulich wrote: >> On 25.08.2022 11:26, Juergen Gross wrote: >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -602,6 +602,10 @@ static int lock_pages( >>> *pinned += page_count; >>> nr_pages -= page_count; >>> pages += page_count; >>> + >>> + /* Exact reason isn't known, EFAULT is one possibility. */ >>> + if (page_count < requested) >>> + return -EFAULT; >>> } >> >> I don't really know the inner workings of pin_user_pages_fast() >> nor what future plans there are with it. To be as independent of >> its behavior as possible, how about bailing here only when >> page_count actually is zero (i.e. no forward progress)? > > This would require to rework the loop in lock_pages() to be able to > handle only a partial buffer. Oh, I see - I've misread the code as if the loop was capping each iteration's count to the capacity of some internal buffer (as iirc is being done elsewhere). So ... > This would add some complexity, but OTOH I'd get an exact error code > back in case of failure. ... perhaps not worth it then, ... > I'll have a try and see how the result would look like. ... unless you think this might be relevant in certain cases. Jan
On 25.08.22 12:22, Jan Beulich wrote: > On 25.08.2022 12:13, Juergen Gross wrote: >> On 25.08.22 11:50, Jan Beulich wrote: >>> On 25.08.2022 11:26, Juergen Gross wrote: >>>> --- a/drivers/xen/privcmd.c >>>> +++ b/drivers/xen/privcmd.c >>>> @@ -602,6 +602,10 @@ static int lock_pages( >>>> *pinned += page_count; >>>> nr_pages -= page_count; >>>> pages += page_count; >>>> + >>>> + /* Exact reason isn't known, EFAULT is one possibility. */ >>>> + if (page_count < requested) >>>> + return -EFAULT; >>>> } >>> >>> I don't really know the inner workings of pin_user_pages_fast() >>> nor what future plans there are with it. To be as independent of >>> its behavior as possible, how about bailing here only when >>> page_count actually is zero (i.e. no forward progress)? >> >> This would require to rework the loop in lock_pages() to be able to >> handle only a partial buffer. > > Oh, I see - I've misread the code as if the loop was capping each > iteration's count to the capacity of some internal buffer (as iirc > is being done elsewhere). So ... > >> This would add some complexity, but OTOH I'd get an exact error code >> back in case of failure. > > ... perhaps not worth it then, ... > >> I'll have a try and see how the result would look like. > > ... unless you think this might be relevant in certain cases. Not sure, but the resulting code is looking fine IMO. Juergen
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 3369734108af..7dc62510635e 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -602,6 +602,10 @@ static int lock_pages( *pinned += page_count; nr_pages -= page_count; pages += page_count; + + /* Exact reason isn't known, EFAULT is one possibility. */ + if (page_count < requested) + return -EFAULT; } 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 patches 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() --- drivers/xen/privcmd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)