* [PATCH 1/1] Fix: vmw_vmci driver get_user_pages_fast error handling
@ 2017-10-31 23:00 Mathieu Desnoyers
2017-11-02 18:46 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: Mathieu Desnoyers @ 2017-10-31 23:00 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-kernel, Mathieu Desnoyers, George Zhang, Andy king,
Dmitry Torokhov, Greg Kroah-Hartman, linux-fsdevel
Comparing a signed return value against an unsigned num_pages
field performs the comparison as "unsigned", and therefore mistakenly
considers get_user_pages_fast() errors as success.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: George Zhang <georgezhang@vmware.com>
CC: Andy king <acking@vmware.com>
CC: Dmitry Torokhov <dtor@vmware.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: linux-fsdevel@vger.kernel.org
---
drivers/misc/vmw_vmci/vmci_queue_pair.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c
index 8af5c2672f71..f71db848ce81 100644
--- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
+++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
@@ -755,7 +755,7 @@ static int qp_host_get_user_memory(u64 produce_uva,
retval = get_user_pages_fast((uintptr_t) produce_uva,
produce_q->kernel_if->num_pages, 1,
produce_q->kernel_if->u.h.header_page);
- if (retval < produce_q->kernel_if->num_pages) {
+ if (retval < 0 || retval < produce_q->kernel_if->num_pages) {
pr_debug("get_user_pages_fast(produce) failed (retval=%d)",
retval);
qp_release_pages(produce_q->kernel_if->u.h.header_page,
@@ -767,7 +767,7 @@ static int qp_host_get_user_memory(u64 produce_uva,
retval = get_user_pages_fast((uintptr_t) consume_uva,
consume_q->kernel_if->num_pages, 1,
consume_q->kernel_if->u.h.header_page);
- if (retval < consume_q->kernel_if->num_pages) {
+ if (retval < 0 || retval < consume_q->kernel_if->num_pages) {
pr_debug("get_user_pages_fast(consume) failed (retval=%d)",
retval);
qp_release_pages(consume_q->kernel_if->u.h.header_page,
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] Fix: vmw_vmci driver get_user_pages_fast error handling
2017-10-31 23:00 [PATCH 1/1] Fix: vmw_vmci driver get_user_pages_fast error handling Mathieu Desnoyers
@ 2017-11-02 18:46 ` Al Viro
2017-11-02 18:54 ` Mathieu Desnoyers
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2017-11-02 18:46 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, George Zhang, Andy king, Dmitry Torokhov,
Greg Kroah-Hartman, linux-fsdevel
On Tue, Oct 31, 2017 at 07:00:38PM -0400, Mathieu Desnoyers wrote:
> Comparing a signed return value against an unsigned num_pages
> field performs the comparison as "unsigned", and therefore mistakenly
> considers get_user_pages_fast() errors as success.
It's worse than that - if you look into the code in question you'll
see
pr_debug("get_user_pages_fast(produce) failed (retval=%d)",
retval);
qp_release_pages(produce_q->kernel_if->u.h.header_page,
retval, false);
err = VMCI_ERROR_NO_MEM;
goto out;
with
static void qp_release_pages(struct page **pages,
u64 num_pages, bool dirty)
{
int i;
for (i = 0; i < num_pages; i++) {
if (dirty)
set_page_dirty(pages[i]);
put_page(pages[i]);
pages[i] = NULL;
}
}
Now, guess what'll happen if you get there with retval being negative?
AFAICS, the right fix is something along the lines of
if (retval != produce_q->kernel_if->num_pages) {
pr_debug("get_user_pages_fast(produce) failed (retval=%d)",
retval);
if (retval > 0)
qp_release_pages(produce_q->kernel_if->u.h.header_page,
retval, false);
err = VMCI_ERROR_NO_MEM;
goto out;
}
and similar for the second caller. Objections?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] Fix: vmw_vmci driver get_user_pages_fast error handling
2017-11-02 18:46 ` Al Viro
@ 2017-11-02 18:54 ` Mathieu Desnoyers
0 siblings, 0 replies; 3+ messages in thread
From: Mathieu Desnoyers @ 2017-11-02 18:54 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-kernel, George Zhang, Andy king, Dmitry Torokhov,
Greg Kroah-Hartman, linux-fsdevel
----- On Nov 2, 2017, at 2:46 PM, Alexander Viro viro@ZenIV.linux.org.uk wrote:
> On Tue, Oct 31, 2017 at 07:00:38PM -0400, Mathieu Desnoyers wrote:
>> Comparing a signed return value against an unsigned num_pages
>> field performs the comparison as "unsigned", and therefore mistakenly
>> considers get_user_pages_fast() errors as success.
>
> It's worse than that - if you look into the code in question you'll
> see
> pr_debug("get_user_pages_fast(produce) failed (retval=%d)",
> retval);
> qp_release_pages(produce_q->kernel_if->u.h.header_page,
> retval, false);
> err = VMCI_ERROR_NO_MEM;
> goto out;
> with
> static void qp_release_pages(struct page **pages,
> u64 num_pages, bool dirty)
> {
> int i;
>
> for (i = 0; i < num_pages; i++) {
> if (dirty)
> set_page_dirty(pages[i]);
>
> put_page(pages[i]);
> pages[i] = NULL;
> }
> }
>
> Now, guess what'll happen if you get there with retval being negative?
Well this ought to be a pretty long loop...
> AFAICS, the right fix is something along the lines of
> if (retval != produce_q->kernel_if->num_pages) {
> pr_debug("get_user_pages_fast(produce) failed (retval=%d)",
> retval);
> if (retval > 0)
> qp_release_pages(produce_q->kernel_if->u.h.header_page,
> retval, false);
> err = VMCI_ERROR_NO_MEM;
> goto out;
> }
> and similar for the second caller. Objections?
No objection from me,
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-02 18:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 23:00 [PATCH 1/1] Fix: vmw_vmci driver get_user_pages_fast error handling Mathieu Desnoyers
2017-11-02 18:46 ` Al Viro
2017-11-02 18:54 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).