[v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config

[v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config

[Arvind Yadav]

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

Re: [v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config

[Michal Nazarewicz]

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»