[PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[Xiyu Yang]

A NULL vmci_ctx object may pass to vmci_ctx_put() from its callers.
Add a NULL check to prevent NULL pointer 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
index 16695366ec92..2f4963ab51bd 100644
--- a/drivers/misc/vmw_vmci/vmci_context.c
+++ b/drivers/misc/vmw_vmci/vmci_context.c
@@ -494,7 +494,8 @@ static void ctx_free_ctx(struct kref *kref)
  */
 void vmci_ctx_put(struct vmci_ctx *context)
 {
-	kref_put(&context->kref, ctx_free_ctx);
+	if (context)
+		kref_put(&context->kref, ctx_free_ctx);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[Greg Kroah-Hartman]

On Mon, Mar 23, 2020 at 04:22:33PM +0800, Xiyu Yang wrote:
> A NULL vmci_ctx object may pass to vmci_ctx_put() from its callers.

Are you sure this can happen?

> Add a NULL check to prevent NULL pointer 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

What changed from v1?

Always put that below the --- line.

Please fix up and send a v3.

thanks,

greg k-h

Re: [PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[Arnd Bergmann]

On Mon, Mar 23, 2020 at 9:52 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 23, 2020 at 04:22:33PM +0800, Xiyu Yang wrote:
> > A NULL vmci_ctx object may pass to vmci_ctx_put() from its callers.
>
> Are you sure this can happen?
>
> > Add a NULL check to prevent NULL pointer dereference.

It looks like this could happen if vmci_ctx_get() returns NULL, which is
not checked for consistently. Maybe add better error handling to the
callers that currently don't check for that, to catch problems such as

void vmci_ctx_rcv_notifications_release(...)
{
        struct vmci_ctx *context = vmci_ctx_get(context_id); /* may be NULL */
       ...
       context->pending_doorbell_array = db_handle_array;
       ...
       vmci_ctx_put(context);
}

Checking only in vmci_ctx_put() is too late.

     Arnd

RE: [PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[Jorgen Hansen]

> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Monday, March 23, 2020 10:17 AM
> On Mon, Mar 23, 2020 at 9:52 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Mar 23, 2020 at 04:22:33PM +0800, Xiyu Yang wrote:
> > > A NULL vmci_ctx object may pass to vmci_ctx_put() from its callers.
> >
> > Are you sure this can happen?
> >
> > > Add a NULL check to prevent NULL pointer dereference.
> 
> It looks like this could happen if vmci_ctx_get() returns NULL, which is
> not checked for consistently. Maybe add better error handling to the
> callers that currently don't check for that, to catch problems such as

In the cases, where the return value isn't checked, the return value of
vmci_ctx_get() won't be NULL, as the code won't be reached unless the
context ID has an associated context structure. In the example below,
the caller has obtained the context_id from an active context. That said,
it wouldn't hurt to add either checks or at least a comment as to why
the context won't be NULL in the cases, where it isn't checked today.

> 
> void vmci_ctx_rcv_notifications_release(...)
> {
>         struct vmci_ctx *context = vmci_ctx_get(context_id); /* may be NULL */
>        ...
>        context->pending_doorbell_array = db_handle_array;
>        ...
>        vmci_ctx_put(context);
> }
> 
> Checking only in vmci_ctx_put() is too late.
> 
>      Arnd

Thanks,
Jorgen

Re: [PATCH v2] VMCI: Fix NULL pointer dereference on context ptr

[Xiyu Yang]

On Mon, Mar 23, 2020 at 09:52:41AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 23, 2020 at 04:22:33PM +0800, Xiyu Yang wrote:
> > A NULL vmci_ctx object may pass to vmci_ctx_put() from its callers.
> 
> Are you sure this can happen?

Yes. We reviewed all callers of vmci_ctx_put(), and confirmed that 
at least 3 callers may pass a NULL vmci_ctx object to vmci_ctx_put().
 
Given the following qp_broker_attach() as an example, we find 
vmci_ctx_get() may return NULL, as confirmed the NULL check performed 
by vmci_ctx_supports_host_qp(). 
Thus, we believe a NULL check for vmci_ctx_put() is also necessary.

void qp_broker_attach(struct qp_broker_entry *entry,...)
{
	...
	create_context = vmci_ctx_get(entry->create_id); /* may be NULL */
	supports_host_qp = vmci_ctx_supports_host_qp(create_context); /* do NULL-check inside */ 
	vmci_ctx_put(create_context);  /* lack NULL-check */ 
	...
}

bool vmci_ctx_supports_host_qp(struct vmci_ctx *context)
{
	/* NULL-check before pointer dereference */
	return context && context->user_version >= VMCI_VERSION_HOSTQP;  
}

void vmci_ctx_put(struct vmci_ctx *context)
{
	/* A potential NULL pointer will be accessed to get context's refcount field */
	kref_put(&context->kref, ctx_free_ctx);
}

Similary situtations are confirmed for other two callers of vmci_ctx_put(): 
qp_detatch_host_work(), qp_alloc_host_work(). 

static int qp_detatch_host_work(struct vmci_handle handle)
{
	int result;
	struct vmci_ctx *context;

	context = vmci_ctx_get(VMCI_HOST_CONTEXT_ID); /* may be NULL */
	result = vmci_qp_broker_detach(handle, context); /* do NULL-check inside */
	vmci_ctx_put(context); /* lack NULL-check */
	return result;
}

static int qp_alloc_host_work(...)
{
	...
	context = vmci_ctx_get(VMCI_HOST_CONTEXT_ID); /* may be NULL */
	...
	result = qp_broker_alloc(...,context,...); /* if context == NULL, result != VMCI_SUCCESS */
	if (result == VMCI_SUCCESS) {
		...
	} else {
		*handle = VMCI_INVALID_HANDLE;
		pr_devel("queue pair broker failed to alloc (result=%d)\n",
			 result);
	}
	vmci_ctx_put(context); /* lack NULL-check */
	return result;
}

Considering vmci_ctx_supports_host_qp() performs an internal 
NULL check on vmci_ctx pointer, is it also appropriate to 
perform the NULL check inside vmci_ctx_put()?

> 
> > Add a NULL check to prevent NULL pointer 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 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> What changed from v1?
> 
> Always put that below the --- line.
> 
> Please fix up and send a v3.
> 
> thanks,
> 
> greg k-h