Messages in this thread | | | Date | Mon, 23 Mar 2020 12:53:02 +0800 | From | Xiyu Yang <> | Subject | Re: Re: [PATCH] VMCI: Fix NULL pointer dereference on context ptr |
| |
Hi Greg,
On Wed, Mar 18, 2020 at 12:02:04PM +0100, Greg Kroah-Hartman wrote: > On Tue, Mar 17, 2020 at 02:29:57PM +0800, Xiyu Yang wrote: > > The refcount wrapper function vmci_ctx_get() may return NULL > > context ptr. Thus, we need to add a NULL pointer check > > before its dereference. > > > > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > > --- > > drivers/misc/vmw_vmci/vmci_context.c | 2 ++ > > drivers/misc/vmw_vmci/vmci_queue_pair.c | 17 +++++++++++------ > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c > > index 16695366ec92..a20878fba374 100644 > > --- a/drivers/misc/vmw_vmci/vmci_context.c > > +++ b/drivers/misc/vmw_vmci/vmci_context.c > > @@ -898,6 +898,8 @@ void vmci_ctx_rcv_notifications_release(u32 context_id, > > bool success) > > { > > struct vmci_ctx *context = vmci_ctx_get(context_id); > > + if (context == NULL) > > + return; > > Same comment as on your other patch. > > And is this a v2?
Thanks! Yes, this is a v2.
According to our observation, vmci_ctx_rcv_notifications_release() currently is only called by vmci_host_do_recv_notifications() which guarantees a valid context object can be acquired with this context_id.
However, we argue that a NULL-check here is still necessary because this function may be called by other functions in the future who may fail/forget to provide such guarantee. A NULL-check could gracely eliminiate such assumption on the callers for vmci_ctx_rcv_notifications_release() with negligible overhead.
> > spin_lock(&context->lock); > > if (!success) { > > diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c > > index 8531ae781195..2ecb22d08692 100644 > > --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c > > +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c > > @@ -1575,11 +1575,14 @@ static int qp_broker_attach(struct qp_broker_entry *entry, > > */ > > > > create_context = vmci_ctx_get(entry->create_id); > > - supports_host_qp = vmci_ctx_supports_host_qp(create_context); > > - vmci_ctx_put(create_context); > > + if (!create_context) { > > + supports_host_qp = > > + vmci_ctx_supports_host_qp(create_context); > > + vmci_ctx_put(create_context); > > > > - if (!supports_host_qp) > > - return VMCI_ERROR_INVALID_RESOURCE; > > + if (!supports_host_qp) > > + return VMCI_ERROR_INVALID_RESOURCE; > > + } > > } > > > > if ((entry->qp.flags & ~VMCI_QP_ASYMM) != (flags & ~VMCI_QP_ASYMM_PEER)) > > @@ -1808,7 +1811,8 @@ static int qp_alloc_host_work(struct vmci_handle *handle, > > pr_devel("queue pair broker failed to alloc (result=%d)\n", > > result); > > } > > - vmci_ctx_put(context); > > + if (context) > > + vmci_ctx_put(context); > > can't vmci_ctx_put() take a NULL pointer? If not, fix this that way > please. > > thanks, > > greg k-h
No. vmci_ctx_put() can not take a NULL pointer. Sure, we will submit a new patch to perform a NULL-check in vmci_ctx_put().
| |