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

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

[Xiyu Yang]

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;
 
 	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);
 	return result;
 }
 
@@ -1859,7 +1863,8 @@ static int qp_detatch_host_work(struct vmci_handle handle)
 
 	result = vmci_qp_broker_detach(handle, context);
 
-	vmci_ctx_put(context);
+	if (context)
+		vmci_ctx_put(context);
 	return result;
 }
 
-- 
2.7.4

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

[Greg Kroah-Hartman]

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?

>  
>  	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

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

[Xiyu Yang]

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().

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

[Greg Kroah-Hartman]

On Mon, Mar 23, 2020 at 12:53:02PM +0800, Xiyu Yang wrote:
> 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. 

No, that's not how we write code in the kernel, if it does not need to
be checked for because this can not happen, then do not check for it.

Don't try to plan for random users of your code in the future when you
control those users directly :)

thanks,

greg k-h

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

[Arnd Bergmann]

On Mon, Mar 23, 2020 at 7:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 23, 2020 at 12:53:02PM +0800, Xiyu Yang wrote:
> > 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:
> > >
> > > 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.
>
> No, that's not how we write code in the kernel, if it does not need to
> be checked for because this can not happen, then do not check for it.
>
> Don't try to plan for random users of your code in the future when you
> control those users directly :)

Just saw this reply after replying to the other mail. I guess I picked
a bad example ;-)

If there was in fact a report about a NULL pointer at put() time
somewhere, that would be the first thing to fix, and it may still
make sense to review the other code paths to see if there
are additional cases that can go wrong.

      Arnd