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 <[email protected]>
Signed-off-by: Xin Tan <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Xin Tan <[email protected]>
> ---
> 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
On Mon, Mar 23, 2020 at 9:52 AM Greg Kroah-Hartman
<[email protected]> 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
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Monday, March 23, 2020 10:17 AM
> On Mon, Mar 23, 2020 at 9:52 AM Greg Kroah-Hartman
> <[email protected]> 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
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 <[email protected]>
> > Signed-off-by: Xin Tan <[email protected]>
> > ---
> > 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