Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Mar 2020 12:53:02 +0800
From:   Xiyu Yang <xiyuyang19@fudan.edu.cn>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Xin Tan <tanxin.ctf@gmail.com>,
        Alexios Zavras <alexios.zavras@intel.com>,
        Vishnu DASA <vdasa@vmware.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Allison Randal <allison@lohutok.net>,
        Ira Weiny <ira.weiny@intel.com>,
        Mike Marshall <hubcap@omnibond.com>,
        linux-kernel@vger.kernel.org, yuanxzhang@fudan.edu.cn, kjlu@umn.edu
Subject: Re: Re: [PATCH] VMCI: Fix NULL pointer dereference on context ptr
Message-ID: <20200323045302.GA117440@sherlly>
References: <1584426607-89366-1-git-send-email-xiyuyang19@fudan.edu.cn>
 <20200318110204.GB2305113@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200318110204.GB2305113@kroah.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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