* Re: firmware: fix possible use after free on name on asynchronous request
2015-05-28 9:02 firmware: fix possible use after free on name on asynchronous request Dan Carpenter
@ 2015-05-29 0:45 ` Luis R. Rodriguez
2015-05-29 7:24 ` walter harms
2015-05-29 16:52 ` Luis R. Rodriguez
2 siblings, 0 replies; 4+ messages in thread
From: Luis R. Rodriguez @ 2015-05-29 0:45 UTC (permalink / raw)
To: kernel-janitors
On Thu, May 28, 2015 at 12:02:27PM +0300, Dan Carpenter wrote:
> Hello Luis R. Rodriguez,
>
> The patch f9692b2699bd: "firmware: fix possible use after free on
> name on asynchronous request" from May 12, 2015, leads to the
> following static checker warning:
>
> drivers/base/firmware_class.c:1311 request_firmware_nowait()
> warn: possible memory leak of 'fw_work'
>
> drivers/base/firmware_class.c
> 1296 int
> 1297 request_firmware_nowait(
> 1298 struct module *module, bool uevent,
> 1299 const char *name, struct device *device, gfp_t gfp, void *context,
> 1300 void (*cont)(const struct firmware *fw, void *context))
> 1301 {
> 1302 struct firmware_work *fw_work;
> 1303
> 1304 fw_work = kzalloc(sizeof(struct firmware_work), gfp);
> 1305 if (!fw_work)
> 1306 return -ENOMEM;
> 1307
> 1308 fw_work->module = module;
> 1309 fw_work->name = kstrdup_const(name, gfp);
> 1310 if (!fw_work->name)
>
> kfree(fw_work).
>
> 1311 return -ENOMEM;
> 1312 fw_work->device = device;
> 1313 fw_work->context = context;
> 1314 fw_work->cont = cont;
> 1315 fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> 1316 (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> 1317
> 1318 if (!try_module_get(module)) {
> 1319 kfree_const(fw_work->name);
> 1320 kfree(fw_work);
> 1321 return -EFAULT;
> 1322 }
> 1323
> 1324 get_device(fw_work->device);
> 1325 INIT_WORK(&fw_work->work, request_firmware_work_func);
> 1326 schedule_work(&fw_work->work);
> 1327 return 0;
> 1328 }
Bleh, thanks, I'm submitting this next:
From 30da66c4bb1da33f1a789099e4b02e479332f4a2 Mon Sep 17 00:00:00 2001
From: "Luis R. Rodriguez" <mcgrof@suse.com>
Date: Thu, 28 May 2015 17:43:30 -0700
Subject: [PATCH] firmware: add missing kfree for work on async call
The recent fix to use kstrdup_const() failed to add a
kfree upon failure of name allocation...
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Kyle McMartin <kyle@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
drivers/base/firmware_class.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8c3aa3c..9c42883 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1307,8 +1307,10 @@ request_firmware_nowait(
fw_work->module = module;
fw_work->name = kstrdup_const(name, gfp);
- if (!fw_work->name)
+ if (!fw_work->name) {
+ kfree(fw_work);
return -ENOMEM;
+ }
fw_work->device = device;
fw_work->context = context;
fw_work->cont = cont;
--
2.3.2.209.gd67f9d5.dirty
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: firmware: fix possible use after free on name on asynchronous request
2015-05-28 9:02 firmware: fix possible use after free on name on asynchronous request Dan Carpenter
2015-05-29 0:45 ` Luis R. Rodriguez
@ 2015-05-29 7:24 ` walter harms
2015-05-29 16:52 ` Luis R. Rodriguez
2 siblings, 0 replies; 4+ messages in thread
From: walter harms @ 2015-05-29 7:24 UTC (permalink / raw)
To: kernel-janitors
Am 29.05.2015 02:45, schrieb Luis R. Rodriguez:
> On Thu, May 28, 2015 at 12:02:27PM +0300, Dan Carpenter wrote:
>> Hello Luis R. Rodriguez,
>>
>> The patch f9692b2699bd: "firmware: fix possible use after free on
>> name on asynchronous request" from May 12, 2015, leads to the
>> following static checker warning:
>>
>> drivers/base/firmware_class.c:1311 request_firmware_nowait()
>> warn: possible memory leak of 'fw_work'
>>
>> drivers/base/firmware_class.c
>> 1296 int
>> 1297 request_firmware_nowait(
>> 1298 struct module *module, bool uevent,
>> 1299 const char *name, struct device *device, gfp_t gfp, void *context,
>> 1300 void (*cont)(const struct firmware *fw, void *context))
>> 1301 {
>> 1302 struct firmware_work *fw_work;
>> 1303
>> 1304 fw_work = kzalloc(sizeof(struct firmware_work), gfp);
>> 1305 if (!fw_work)
>> 1306 return -ENOMEM;
>> 1307
>> 1308 fw_work->module = module;
>> 1309 fw_work->name = kstrdup_const(name, gfp);
>> 1310 if (!fw_work->name)
>>
>> kfree(fw_work).
>>
>> 1311 return -ENOMEM;
>> 1312 fw_work->device = device;
>> 1313 fw_work->context = context;
>> 1314 fw_work->cont = cont;
>> 1315 fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
>> 1316 (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>> 1317
>> 1318 if (!try_module_get(module)) {
>> 1319 kfree_const(fw_work->name);
>> 1320 kfree(fw_work);
>> 1321 return -EFAULT;
>> 1322 }
>> 1323
>> 1324 get_device(fw_work->device);
>> 1325 INIT_WORK(&fw_work->work, request_firmware_work_func);
>> 1326 schedule_work(&fw_work->work);
>> 1327 return 0;
>> 1328 }
>
> Bleh, thanks, I'm submitting this next:
>
>>From 30da66c4bb1da33f1a789099e4b02e479332f4a2 Mon Sep 17 00:00:00 2001
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> Date: Thu, 28 May 2015 17:43:30 -0700
> Subject: [PATCH] firmware: add missing kfree for work on async call
>
> The recent fix to use kstrdup_const() failed to add a
> kfree upon failure of name allocation...
>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Cc: Kyle McMartin <kyle@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
> drivers/base/firmware_class.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8c3aa3c..9c42883 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1307,8 +1307,10 @@ request_firmware_nowait(
>
> fw_work->module = module;
> fw_work->name = kstrdup_const(name, gfp);
> - if (!fw_work->name)
> + if (!fw_work->name) {
> + kfree(fw_work);
> return -ENOMEM;
> + }
> fw_work->device = device;
> fw_work->context = context;
> fw_work->cont = cont;
Hi Luis,
if it is possible to change firmware_work
and make char *name a name[] you could alloc via.
kzalloc(sizeof(struct firmware_work)+strlen(name)+1, gfp);
perhaps that zero length can make thinks more easy.
(at least you need only one free).
hope that helps,
re,
wh
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: firmware: fix possible use after free on name on asynchronous request
2015-05-28 9:02 firmware: fix possible use after free on name on asynchronous request Dan Carpenter
2015-05-29 0:45 ` Luis R. Rodriguez
2015-05-29 7:24 ` walter harms
@ 2015-05-29 16:52 ` Luis R. Rodriguez
2 siblings, 0 replies; 4+ messages in thread
From: Luis R. Rodriguez @ 2015-05-29 16:52 UTC (permalink / raw)
To: kernel-janitors
On Fri, May 29, 2015 at 09:24:39AM +0200, walter harms wrote:
>
>
> Am 29.05.2015 02:45, schrieb Luis R. Rodriguez:
> > On Thu, May 28, 2015 at 12:02:27PM +0300, Dan Carpenter wrote:
> >> Hello Luis R. Rodriguez,
> >>
> >> The patch f9692b2699bd: "firmware: fix possible use after free on
> >> name on asynchronous request" from May 12, 2015, leads to the
> >> following static checker warning:
> >>
> >> drivers/base/firmware_class.c:1311 request_firmware_nowait()
> >> warn: possible memory leak of 'fw_work'
> >>
> >> drivers/base/firmware_class.c
> >> 1296 int
> >> 1297 request_firmware_nowait(
> >> 1298 struct module *module, bool uevent,
> >> 1299 const char *name, struct device *device, gfp_t gfp, void *context,
> >> 1300 void (*cont)(const struct firmware *fw, void *context))
> >> 1301 {
> >> 1302 struct firmware_work *fw_work;
> >> 1303
> >> 1304 fw_work = kzalloc(sizeof(struct firmware_work), gfp);
> >> 1305 if (!fw_work)
> >> 1306 return -ENOMEM;
> >> 1307
> >> 1308 fw_work->module = module;
> >> 1309 fw_work->name = kstrdup_const(name, gfp);
> >> 1310 if (!fw_work->name)
> >>
> >> kfree(fw_work).
> >>
> >> 1311 return -ENOMEM;
> >> 1312 fw_work->device = device;
> >> 1313 fw_work->context = context;
> >> 1314 fw_work->cont = cont;
> >> 1315 fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> >> 1316 (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> >> 1317
> >> 1318 if (!try_module_get(module)) {
> >> 1319 kfree_const(fw_work->name);
> >> 1320 kfree(fw_work);
> >> 1321 return -EFAULT;
> >> 1322 }
> >> 1323
> >> 1324 get_device(fw_work->device);
> >> 1325 INIT_WORK(&fw_work->work, request_firmware_work_func);
> >> 1326 schedule_work(&fw_work->work);
> >> 1327 return 0;
> >> 1328 }
> >
> > Bleh, thanks, I'm submitting this next:
> >
> >>From 30da66c4bb1da33f1a789099e4b02e479332f4a2 Mon Sep 17 00:00:00 2001
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > Date: Thu, 28 May 2015 17:43:30 -0700
> > Subject: [PATCH] firmware: add missing kfree for work on async call
> >
> > The recent fix to use kstrdup_const() failed to add a
> > kfree upon failure of name allocation...
> >
> > Cc: Ming Lei <ming.lei@canonical.com>
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Cc: Kyle McMartin <kyle@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> > drivers/base/firmware_class.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8c3aa3c..9c42883 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -1307,8 +1307,10 @@ request_firmware_nowait(
> >
> > fw_work->module = module;
> > fw_work->name = kstrdup_const(name, gfp);
> > - if (!fw_work->name)
> > + if (!fw_work->name) {
> > + kfree(fw_work);
> > return -ENOMEM;
> > + }
> > fw_work->device = device;
> > fw_work->context = context;
> > fw_work->cont = cont;
>
>
> Hi Luis,
> if it is possible to change firmware_work
> and make char *name a name[] you could alloc via.
>
> kzalloc(sizeof(struct firmware_work)+strlen(name)+1, gfp);
>
> perhaps that zero length can make thinks more easy.
> (at least you need only one free).
Indeed, that is how we used to condify this but this was recently changed since
we will be using two strings within the struct, since we have a slab cache
for file names though its then best to just use that as other filesystem
code does as well. By using kstrdup_const() we then also get the gain
to not have to allocate anything if the caller did the right thing to
use .rodata on the kernel.
Luis
^ permalink raw reply [flat|nested] 4+ messages in thread