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@xxxxxxxxxxxx>
> > Signed-off-by: Xin Tan <tanxin.ctf@xxxxxxxxx>
> > ---
> >  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().