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

[Xiyu Yang <xiyuyang19@fudan.edu.cn>]

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