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:   Sun, 29 Mar 2020 20:08:22 +0800
From:   Xiyu Yang <xiyuyang19@fudan.edu.cn>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Alexios Zavras <alexios.zavras@intel.com>,
        Allison Randal <allison@lohutok.net>,
        Adit Ranadive <aditr@vmware.com>,
        Jorgen Hansen <jhansen@vmware.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vishnu DASA <vdasa@vmware.com>, linux-kernel@vger.kernel.org,
        yuanxzhang@fudan.edu.cn, kjlu@umn.edu,
        Xin Tan <tanxin.ctf@gmail.com>
Subject: Re: [PATCH v2] VMCI: Fix NULL pointer dereference on context ptr
Message-ID: <20200329120822.GA7610@sherlly>
References: <1584951832-120773-1-git-send-email-xiyuyang19@fudan.edu.cn>
 <20200323085241.GA342330@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200323085241.GA342330@kroah.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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