* Re: [v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config
2016-12-08 16:32 [v1] usb:gadget:legacy:nokia :- Check for NULL in nokia_bind_config Arvind Yadav
@ 2016-12-09 14:59 ` Michal Nazarewicz
0 siblings, 0 replies; 2+ messages in thread
From: Michal Nazarewicz @ 2016-12-09 14:59 UTC (permalink / raw)
To: Arvind Yadav, balbi, gregkh
Cc: k.opasiak, i.kotrasinsk, linux-usb, linux-kernel
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»
^ permalink raw reply [flat|nested] 2+ messages in thread