From: Michal Nazarewicz <mina86@mina86.com>
To: Arvind Yadav <arvind.yadav.cs@gmail.com>, balbi@kernel.org,
        gregkh@linuxfoundation.org
Cc: k.opasiak@samsung.com, i.kotrasinsk@samsung.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config
In-Reply-To: <1481214733-8702-1-git-send-email-arvind.yadav.cs@gmail.com>
Organization: http://mina86.com/
References: <1481214733-8702-1-git-send-email-arvind.yadav.cs@gmail.com>
User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/26.0.50.2 (x86_64-unknown-linux-gnu)
Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII=
Date: Fri, 09 Dec 2016 15:59:17 +0100
Message-ID: <xa1tfulxkv22.fsf@mina86.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 4889
Lines: 147

On Thu, Dec 08 2016, Arvind Yadav wrote:
> Here, f_acm,f_ecm and f_msg needs to be checked for being NULL
> in nokia_bind_config() before calling usb_add_function(),
> otherwise kernel can run into a NULL-pointer dereference.
>
> f_phonet, f_obex1 and f_obex2 need to be checked for NULL
> in nokia_bind_config() to print proper debug information.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>

Is this something you’ve encountered?  As far as I can see, NULL is
never returned from usb_get_function.  It’s always an error pointer.

In fact, if usb_get_function returns NULL, then the null pointer
dereference happens *inside* of the function so checking it outside is
too late.

If we even want to worry about it, this needs to be done in functions.c:

----------- >8 ---------------------------------------------------------
>From 083a31c827b9e931a0e2d1d0b121fdfeccea7ace Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@mina86.com>
Date: Fri, 9 Dec 2016 15:56:44 +0100
Subject: [PATCH] =?UTF-8?q?usb:=20function:=20make=20sure=20usb=5Fget=5Ffu?=
 =?UTF-8?q?nction*=20functions=20don=E2=80=99t=20returen=20NULL?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The interface for usb_get_function and usb_get_function_instance is
that they return a pointer or an error pointer.  In other words, they
never return NULL.  For that to work, they rely on alloc_func and
alloc_inst callbacks respectively to never return NULL either.  Indeed,
if those callbacks ever return NULL, a null pointer dereference will
happen.

Be defensive about it and check in those functions whether the callbacks
(incorrectly) returned NULL and interpret it as ENOMEM error pointer
instead.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/functions.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/functions.c b/drivers/usb/gadget/functions.c
index b13f839..4a1f1fb 100644
--- a/drivers/usb/gadget/functions.c
+++ b/drivers/usb/gadget/functions.c
@@ -27,12 +27,12 @@ static struct usb_function_instance *try_get_usb_function_instance(const char *n
 		fi = fd->alloc_inst();
 		if (IS_ERR(fi))
 			module_put(fd->mod);
-		else
+		else if (fi)  /* This should always be true but be defensive. */
 			fi->fd = fd;
 		break;
 	}
 	mutex_unlock(&func_lock);
-	return fi;
+	return fi ? fi : ERR_PTR(-ENOMEM);
 }
 
 struct usb_function_instance *usb_get_function_instance(const char *name)
@@ -43,6 +43,8 @@ struct usb_function_instance *usb_get_function_instance(const char *name)
 	fi = try_get_usb_function_instance(name);
 	if (!IS_ERR(fi))
 		return fi;
+	else if (!fi)  /* We should never be here but be defensive about it. */
+		return ERR_PTR(-ENOMEM);
 	ret = PTR_ERR(fi);
 	if (ret != -ENOENT)
 		return fi;
@@ -60,6 +62,8 @@ struct usb_function *usb_get_function(struct usb_function_instance *fi)
 	f = fi->fd->alloc_func(fi);
 	if (IS_ERR(f))
 		return f;
+	else if (!f)  /* We should never be here but be defensive about it. */
+		return ERR_PTR(-ENOMEM);
 	f->fi = fi;
 	return f;
 }
-- 
2.8.0.rc3.226.g39d4020
----------- >8 ---------------------------------------------------------

However, I don’t think it is necessary and it only complicates code.

> ---
>  drivers/usb/gadget/legacy/nokia.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c
> index b1e535f..d3ba286 100644
> --- a/drivers/usb/gadget/legacy/nokia.c
> +++ b/drivers/usb/gadget/legacy/nokia.c
> @@ -159,36 +159,36 @@ static int nokia_bind_config(struct usb_configuration *c)
>  
>  	if (!IS_ERR(fi_phonet)) {
>  		f_phonet = usb_get_function(fi_phonet);
> -		if (IS_ERR(f_phonet))
> +		if (IS_ERR_OR_NULL(f_phonet))
>  			pr_debug("could not get phonet function\n");
>  	}
>  
>  	if (!IS_ERR(fi_obex1)) {
>  		f_obex1 = usb_get_function(fi_obex1);
> -		if (IS_ERR(f_obex1))
> +		if (IS_ERR_OR_NULL(f_obex1))
>  			pr_debug("could not get obex function 0\n");
>  	}
>  
>  	if (!IS_ERR(fi_obex2)) {
>  		f_obex2 = usb_get_function(fi_obex2);
> -		if (IS_ERR(f_obex2))
> +		if (IS_ERR_OR_NULL(f_obex2))
>  			pr_debug("could not get obex function 1\n");
>  	}
>  
>  	f_acm = usb_get_function(fi_acm);
> -	if (IS_ERR(f_acm)) {
> +	if (IS_ERR_OR_NULL(f_acm)) {
>  		status = PTR_ERR(f_acm);
>  		goto err_get_acm;
>  	}
>  
>  	f_ecm = usb_get_function(fi_ecm);
> -	if (IS_ERR(f_ecm)) {
> +	if (IS_ERR_OR_NULL(f_ecm)) {
>  		status = PTR_ERR(f_ecm);
>  		goto err_get_ecm;
>  	}
>  
>  	f_msg = usb_get_function(fi_msg);
> -	if (IS_ERR(f_msg)) {
> +	if (IS_ERR_OR_NULL(f_msg)) {
>  		status = PTR_ERR(f_msg);
>  		goto err_get_msg;
>  	}
> -- 
> 2.7.4
>

-- 
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»