[PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Chen Zhou]

In gb_lights_light_config(), 'light->name' is allocated by kstrndup().
It returns NULL when fails, add check for it.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
---
 drivers/staging/greybus/light.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index d6ba25f..d2672b6 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
 
 	light->channels_count = conf.channel_count;
 	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
-
+	if (!light->name)
+		return -ENOMEM;
 	light->channels = kcalloc(light->channels_count,
 				  sizeof(struct gb_channel), GFP_KERNEL);
 	if (!light->channels)
-- 
2.7.4

Re: [PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Rui Miguel Silva]

Hi Chen Zhou,
Thanks for the patch.

On Wed, Apr 01, 2020 at 11:00:17AM +0800, Chen Zhou wrote:
> In gb_lights_light_config(), 'light->name' is allocated by kstrndup().
> It returns NULL when fails, add check for it.
> 
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

> ---
>  drivers/staging/greybus/light.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index d6ba25f..d2672b6 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
>  
>  	light->channels_count = conf.channel_count;
>  	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
> -
> +	if (!light->name)
> +		return -ENOMEM;
>  	light->channels = kcalloc(light->channels_count,
>  				  sizeof(struct gb_channel), GFP_KERNEL);
>  	if (!light->channels)
> -- 
> 2.7.4
> 

Re: [PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Dan Carpenter]

On Wed, Apr 01, 2020 at 11:00:17AM +0800, Chen Zhou wrote:
> In gb_lights_light_config(), 'light->name' is allocated by kstrndup().
> It returns NULL when fails, add check for it.
> 
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> ---
>  drivers/staging/greybus/light.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index d6ba25f..d2672b6 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
>  
>  	light->channels_count = conf.channel_count;
>  	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
> -
> +	if (!light->name)
> +		return -ENOMEM;
>  	light->channels = kcalloc(light->channels_count,
>  				  sizeof(struct gb_channel), GFP_KERNEL);
>  	if (!light->channels)

The clean up in this function is non-existant.  :(

regards,
dan carpenter

Re: [PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Rui Miguel Silva]

Hi Dan,

On Thu, Apr 02, 2020 at 03:22:28PM +0300, Dan Carpenter wrote:
> On Wed, Apr 01, 2020 at 11:00:17AM +0800, Chen Zhou wrote:
> > In gb_lights_light_config(), 'light->name' is allocated by kstrndup().
> > It returns NULL when fails, add check for it.
> > 
> > Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> > ---
> >  drivers/staging/greybus/light.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > index d6ba25f..d2672b6 100644
> > --- a/drivers/staging/greybus/light.c
> > +++ b/drivers/staging/greybus/light.c
> > @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
> >  
> >  	light->channels_count = conf.channel_count;
> >  	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
> > -
> > +	if (!light->name)
> > +		return -ENOMEM;
> >  	light->channels = kcalloc(light->channels_count,
> >  				  sizeof(struct gb_channel), GFP_KERNEL);
> >  	if (!light->channels)
> 
> The clean up in this function is non-existant.  :(

Yeah, this have a central point to do the cleanups, gb_lights_release,
since we may have other lights already configured at this point, we
could cleanup this specific one here, but than would need to make sure
all other already configure got clean also.

------
Cheers,
     Rui

Re: [PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Dan Carpenter]

On Thu, Apr 02, 2020 at 02:16:18PM +0100, Rui Miguel Silva wrote:
> > > --- a/drivers/staging/greybus/light.c
> > > +++ b/drivers/staging/greybus/light.c
> > > @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
> > >  
> > >  	light->channels_count = conf.channel_count;
> > >  	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
> > > -
> > > +	if (!light->name)
> > > +		return -ENOMEM;
> > >  	light->channels = kcalloc(light->channels_count,
> > >  				  sizeof(struct gb_channel), GFP_KERNEL);
> > >  	if (!light->channels)
> > 
> > The clean up in this function is non-existant.  :(
> 
> Yeah, this have a central point to do the cleanups, gb_lights_release,
> since we may have other lights already configured at this point, we
> could cleanup this specific one here, but than would need to make sure
> all other already configure got clean also.

Central clean up functions never work correctly.

For example, we allocate "cdev->name" in gb_lights_channel_config()
before we register the channel later in gb_lights_register_all(glights);.
Now imagine that the register fails.  Then when we're freeing it in
__gb_lights_led_unregister() we see that the ->is_registered is false
so we don't kfree(cdev->name).

That's just a small memory leak.  But there are going to be tons of
little bugs like that.

Anyway it doesn't affect this patch so it's fine.

regards,
dan carpenter

Re: [PATCH -next] staging: greybus: fix a missing-check bug in gb_lights_light_config()

[Rui Miguel Silva]

Hi,
On Thu, Apr 02, 2020 at 05:22:37PM +0300, Dan Carpenter wrote:
> On Thu, Apr 02, 2020 at 02:16:18PM +0100, Rui Miguel Silva wrote:
> > > > --- a/drivers/staging/greybus/light.c
> > > > +++ b/drivers/staging/greybus/light.c
> > > > @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id)
> > > >  
> > > >  	light->channels_count = conf.channel_count;
> > > >  	light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL);
> > > > -
> > > > +	if (!light->name)
> > > > +		return -ENOMEM;
> > > >  	light->channels = kcalloc(light->channels_count,
> > > >  				  sizeof(struct gb_channel), GFP_KERNEL);
> > > >  	if (!light->channels)
> > > 
> > > The clean up in this function is non-existant.  :(
> > 
> > Yeah, this have a central point to do the cleanups, gb_lights_release,
> > since we may have other lights already configured at this point, we
> > could cleanup this specific one here, but than would need to make sure
> > all other already configure got clean also.
> 
> Central clean up functions never work correctly.

I agree.

> 
> For example, we allocate "cdev->name" in gb_lights_channel_config()
> before we register the channel later in gb_lights_register_all(glights);.
> Now imagine that the register fails.  Then when we're freeing it in
> __gb_lights_led_unregister() we see that the ->is_registered is false
> so we don't kfree(cdev->name).
> 
> That's just a small memory leak.  But there are going to be tons of
> little bugs like that.

Yeah, when I have some cycles I'll go over that error codes paths and
mitigate this kind of issues.

> 
> Anyway it doesn't affect this patch so it's fine.

Yeah, thanks.

------
Cheers,
     Rui