From: "Luis R. Rodriguez" <[email protected]>
This is a follow up to my original series that added kernel firmware
signature check support. That series was split into 3 parts, one which
had fixes, a second set which added firmware signature support, and
a last set which provided system data firmware support as a spring
cleaning effort on the firmware_class driver API. During review I've
spotted even more fixes required on firmware_class, because of this
and in order to help make the review easier I'm splitting the series
out completely. This series only addresses fixes and enhancements for
firmware_class. When reviewing these please keep in mind that one of
the end goals here is to eventually add address firmware signature support,
this means we want to be pretty pedantic and careful about how we handle
names and files.
I've removed Cc: stable notations because although they are fixes they
don't really fix any reported issues even though I can trigger at least
one panic on demand, I'll let Greg and others decide what patches should
be merged in and trickled down to stable. Its not an easy judgement call,
and because of this I've tried to split out fixes out as atomically as
possible. If its of any help I think the Patch 1-2, 4, should all go
in to stable while Patch 3, 5 can be considered optimizations which are
not really stable fixes.
Luis R. Rodriguez (5):
firmware: fix __getname() missing failure check
firmware: check for file truncation on direct firmware loading
firmware: check for possible file truncation early
firmware: fix possible use after free on name on asynchronous request
firmware: use const for remaining firmware names
drivers/base/firmware_class.c | 110 ++++++++++++++++++++++++++++++++++--------
1 file changed, 91 insertions(+), 19 deletions(-)
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
The request_firmware*() APIs uses __getname() to iterate
over the list of paths possible for firmware to be found,
the code however never checked for failure on __getname().
Although *very unlikely*, this can still happen. Add the
missing check.
There is still no checks on the concatenation of the path
and filename passed, that requires a bit more work and
subsequent patches address this. The commit that introduced
this is abb139e7 ("firmware: teach the kernel to load
firmware files directly from the filesystem").
mcgrof@ergon ~/linux (git::firmware-fixes) $ git describe --contains abb139e7
v3.7-rc1~120
Cc: Linus Torvalds <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kyle McMartin <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841a..bc6c8e6 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device,
{
int i;
int rc = -ENOENT;
- char *path = __getname();
+ char *path;
+
+ path = __getname();
+ if (unlikely(!path))
+ return PTR_ERR(path);
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
struct file *file;
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
When direct firmware loading is used we iterate over a list
of possible firmware paths and concatenate the desired firmware
name with each path and look for the file there. Should the
passed firmware name be too long we end up truncating the
file we want to look for, the search however is still done.
Add a check for truncation instead of looking for a
truncated firmware filename.
Cc: Linus Torvalds <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kyle McMartin <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bc6c8e6..99385fc 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -320,7 +320,7 @@ fail:
static int fw_get_filesystem_firmware(struct device *device,
struct firmware_buf *buf)
{
- int i;
+ int i, len;
int rc = -ENOENT;
char *path;
@@ -335,7 +335,12 @@ static int fw_get_filesystem_firmware(struct device *device,
if (!fw_path[i][0])
continue;
- snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
+ len = snprintf(path, PATH_MAX, "%s/%s",
+ fw_path[i], buf->fw_id);
+ if (len >= PATH_MAX) {
+ rc = -ENAMETOOLONG;
+ break;
+ }
file = filp_open(path, O_RDONLY, 0);
if (IS_ERR(file))
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
Instead of waiting until the last second to fail
on a request_firmware*() calls due to filename
truncation we can do an early check upon boot
and immediatley avoid any possible issues upfront.
Cc: Linus Torvalds <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kyle McMartin <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 49 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 99385fc..448388b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -38,6 +38,20 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
MODULE_DESCRIPTION("Multi purpose firmware loading support");
MODULE_LICENSE("GPL");
+static size_t __read_mostly max_sysdata_path_size;
+
+static int sysdata_validate_filename(const char *name)
+{
+ if (!name)
+ return -EINVAL;
+ /* POSIX.1 2.4: an empty pathname is invalid, match other checks */
+ if (name[0] == '\0')
+ return -ENOENT;
+ if (strlen(name) > (PATH_MAX - max_sysdata_path_size))
+ return -ENAMETOOLONG;
+ return 0;
+}
+
/* Builtin firmware support */
#ifdef CONFIG_FW_LOADER
@@ -1105,9 +1119,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
if (!firmware_p)
return -EINVAL;
- if (!name || name[0] == '\0')
- return -EINVAL;
-
ret = _request_firmware_prepare(&fw, name, device);
if (ret <= 0) /* error or already assigned */
goto out;
@@ -1185,6 +1196,10 @@ request_firmware(const struct firmware **firmware_p, const char *name,
{
int ret;
+ ret = sysdata_validate_filename(name);
+ if (ret)
+ return ret;
+
/* Need to pin this module until return */
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device,
@@ -1210,6 +1225,10 @@ int request_firmware_direct(const struct firmware **firmware_p,
{
int ret;
+ ret = sysdata_validate_filename(name);
+ if (ret)
+ return ret;
+
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device,
FW_OPT_UEVENT | FW_OPT_NO_WARN);
@@ -1288,8 +1307,13 @@ request_firmware_nowait(
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context))
{
+ int ret;
struct firmware_work *fw_work;
+ ret = sysdata_validate_filename(name);
+ if (ret)
+ return ret;
+
fw_work = kzalloc(sizeof(struct firmware_work), gfp);
if (!fw_work)
return -ENOMEM;
@@ -1668,8 +1692,27 @@ static void __init fw_cache_init(void)
#endif
}
+static size_t __init get_max_sysdata_path(void)
+{
+ size_t max_size = 0;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+ size_t path_size;
+ /* skip the unset customized path */
+ if (!fw_path[i][0])
+ continue;
+ path_size = strlen(fw_path[i]);
+ if (path_size > max_size)
+ max_size = path_size;
+ }
+
+ return max_size;
+}
+
static int __init firmware_class_init(void)
{
+ max_sysdata_path_size = get_max_sysdata_path();
fw_cache_init();
#ifdef CONFIG_FW_LOADER_USER_HELPER
register_reboot_notifier(&fw_shutdown_nb);
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
Asynchronous firmware loading copies the pointer to the
name passed as an argument only to be scheduled later and
used. This behaviour works well for synchronous calling
but in asynchronous mode there's a chance the caller could
immediately free the passed string after making the
asynchronous call. This could trigger a use after free
having the kernel look on disk for arbitrary file names.
In order to force-test the issue you can use a test-driver
designed to illustrate this issue on github [0], use the
next-20150505-fix-use-after-free branch.
With this patch applied you get:
[ 283.512445] firmware name: test_module_stuff.bin
[ 287.514020] firmware name: test_module_stuff.bin
[ 287.532489] firmware found
Without this patch applied you can end up with something such as:
[ 135.624216] firmware name: \xffffff80BJ
[ 135.624249] platform fake-dev.0: Direct firmware load for \xffffff80Bi failed with error -2
[ 135.624252] No firmware found
[ 135.624252] firmware found
Unfortunatley in the worst and most common case however you
can typically crash your system with a page fault by trying to
free something which you cannot, and/or a NULL pointer
dereference [1].
The fix and issue using schedule_work() for asynchronous
runs is generalized in the following SmPL grammar patch,
when applied to next-20150505 only the firmware_class
code is affected. This grammar patch can and should further
be generalized to vet for for other kernel asynchronous
mechanisms.
@ calls_schedule_work @
type T;
T *priv_work;
identifier func, work_func;
identifier work;
identifier priv_name, name;
expression gfp;
@@
func(..., const char *name, ...)
{
...
priv_work = kzalloc(sizeof(T), gfp);
...
- priv_work->priv_name = name;
+ priv_work->priv_name = kstrdup_const(name, gfp);
...
(... when any
if (...)
{
...
+ kfree_const(priv_work->priv_name);
kfree(priv_work);
...
}
) ... when any
INIT_WORK(&priv_work->work, work_func);
...
schedule_work(&priv_work->work);
...
}
@ the_work_func depends on calls_schedule_work @
type calls_schedule_work.T;
T *priv_work;
identifier calls_schedule_work.work_func;
identifier calls_schedule_work.priv_name;
identifier calls_schedule_work.work;
identifier some_work;
@@
work_func(...)
{
...
priv_work = container_of(some_work, T, work);
...
+ kfree_const(priv_work->priv_name);
kfree(priv_work);
...
}
[0] https://github.com/mcgrof/fake-firmware-test.git
[1] The following kernel ring buffer splat:
firmware name: test_module_stuff.bin
firmware name:
firmware found
general protection fault: 0000 [#1] SMP
Modules linked in: test(O) <...etc-it-does-not-matter>
drm sr_mod cdrom xhci_pci xhci_hcd rtsx_pci mfd_core video button sg
CPU: 3 PID: 87 Comm: kworker/3:2 Tainted: G O 4.0.0-00010-g22b5bb0-dirty #176
Hardware name: LENOVO 20AW000LUS/20AW000LUS, BIOS GLET43WW (1.18 ) 12/04/2013
Workqueue: events request_firmware_work_func
task: ffff8800c7f8e290 ti: ffff8800c7f94000 task.ti: ffff8800c7f94000
RIP: 0010:[<ffffffff814a586c>] [<ffffffff814a586c>] fw_free_buf+0xc/0x40
RSP: 0000:ffff8800c7f97d78 EFLAGS: 00010286
RAX: ffffffff81ae3700 RBX: ffffffff816d1181 RCX: 0000000000000006
RDX: 0001ee850ff68500 RSI: 0000000000000246 RDI: c35d5f415e415d41
RBP: ffff8800c7f97d88 R08: 000000000000000a R09: 0000000000000000
R10: 0000000000000358 R11: ffff8800c7f97a7e R12: ffff8800c7ec1e80
R13: ffff88021e2d4cc0 R14: ffff88021e2dff00 R15: 00000000000000c0
FS: 0000000000000000(0000) GS:ffff88021e2c0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000034b8cd8 CR3: 000000021073c000 CR4: 00000000001407e0
Stack:
ffffffff816d1181 ffff8800c7ec1e80 ffff8800c7f97da8 ffffffff814a58f8
000000000000000a ffffffff816d1181 ffff8800c7f97dc8 ffffffffa047002c
ffff88021e2dff00 ffff8802116ac1c0 ffff8800c7f97df8 ffffffff814a65fe
Call Trace:
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffff814a58f8>] release_firmware+0x58/0x80
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffffa047002c>] test_mod_cb+0x2c/0x43 [test]
[<ffffffff814a65fe>] request_firmware_work_func+0x5e/0x80
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffff8108d23a>] process_one_work+0x14a/0x3f0
[<ffffffff8108d911>] worker_thread+0x121/0x460
[<ffffffff8108d7f0>] ? rescuer_thread+0x310/0x310
[<ffffffff810928f9>] kthread+0xc9/0xe0
[<ffffffff81092830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816d52d8>] ret_from_fork+0x58/0x90
[<ffffffff81092830>] ? kthread_create_on_node+0x180/0x180
Code: c7 c6 dd ad a3 81 48 c7 c7 20 97 ce 81 31 c0 e8 0b b2 ed ff e9 78 ff ff ff 66 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 54 53 <4c> 8b 67 38 48 89 fb 4c 89 e7 e8 85 f7 22 00 f0 83 2b 01 74 0f
RIP [<ffffffff814a586c>] fw_free_buf+0xc/0x40
RSP <ffff8800c7f97d78>
---[ end trace 4e62c56a58d0eac1 ]---
BUG: unable to handle kernel paging request at ffffffffffffffd8
IP: [<ffffffff81093ee0>] kthread_data+0x10/0x20
PGD 1c13067 PUD 1c15067 PMD 0
Oops: 0000 [#2] SMP
Modules linked in: test(O) <...etc-it-does-not-matter>
drm sr_mod cdrom xhci_pci xhci_hcd rtsx_pci mfd_core video button sg
CPU: 3 PID: 87 Comm: kworker/3:2 Tainted: G D O 4.0.0-00010-g22b5bb0-dirty #176
Hardware name: LENOVO 20AW000LUS/20AW000LUS, BIOS GLET43WW (1.18 ) 12/04/2013
task: ffff8800c7f8e290 ti: ffff8800c7f94000 task.ti: ffff8800c7f94000
RIP: 0010:[<ffffffff81092ee0>] [<ffffffff81092ee0>] kthread_data+0x10/0x20
RSP: 0018:ffff8800c7f97b18 EFLAGS: 00010096
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 000000000000000d
RDX: 0000000000000003 RSI: 0000000000000003 RDI: ffff8800c7f8e290
RBP: ffff8800c7f97b18 R08: 000000000000bc00 R09: 0000000000007e76
R10: 0000000000000001 R11: 000000000000002f R12: ffff8800c7f8e290
R13: 00000000000154c0 R14: 0000000000000003 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff88021e2c0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000028 CR3: 0000000210675000 CR4: 00000000001407e0
Stack:
ffff8800c7f97b38 ffffffff8108dcd5 ffff8800c7f97b38 ffff88021e2d54c0
ffff8800c7f97b88 ffffffff816d1500 ffff880213d42368 ffff8800c7f8e290
ffff8800c7f97b88 ffff8800c7f97fd8 ffff8800c7f8e710 0000000000000246
Call Trace:
[<ffffffff8108dcd5>] wq_worker_sleeping+0x15/0xa0
[<ffffffff816d1500>] __schedule+0x6e0/0x940
[<ffffffff816d1797>] schedule+0x37/0x90
[<ffffffff810779bc>] do_exit+0x6bc/0xb40
[<ffffffff8101898f>] oops_end+0x9f/0xe0
[<ffffffff81018efb>] die+0x4b/0x70
[<ffffffff81015622>] do_general_protection+0xe2/0x170
[<ffffffff816d74e8>] general_protection+0x28/0x30
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffff814a586c>] ? fw_free_buf+0xc/0x40
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffff814a58f8>] release_firmware+0x58/0x80
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffffa047002c>] test_mod_cb+0x2c/0x43 [test]
[<ffffffff814a65fe>] request_firmware_work_func+0x5e/0x80
[<ffffffff816d1181>] ? __schedule+0x361/0x940
[<ffffffff8108d23a>] process_one_work+0x14a/0x3f0
[<ffffffff8108d911>] worker_thread+0x121/0x460
[<ffffffff8108d7f0>] ? rescuer_thread+0x310/0x310
[<ffffffff810928f9>] kthread+0xc9/0xe0
[<ffffffff81092830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816d52d8>] ret_from_fork+0x58/0x90
[<ffffffff81092830>] ? kthread_create_on_node+0x180/0x180
Code: 00 48 89 e5 5d 48 8b 40 c8 48 c1 e8 02 83 e0 01 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 8b 87 30 05 00 00 55 48 89 e5 <48> 8b 40 d8 5d c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00
RIP [<ffffffff81092ee0>] kthread_data+0x10/0x20
RSP <ffff8800c7f97b18>
CR2: ffffffffffffffd8
---[ end trace 4e62c56a58d0eac2 ]---
Fixing recursive fault but reboot is needed!
Cc: Linus Torvalds <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: David Howells <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Seth Forshee <[email protected]>
Cc: Kyle McMartin <[email protected]>
Generated-by: Coccinelle SmPL
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 448388b..ae1ed56 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1275,6 +1275,7 @@ static void request_firmware_work_func(struct work_struct *work)
put_device(fw_work->device); /* taken in request_firmware_nowait() */
module_put(fw_work->module);
+ kfree_const(fw_work->name);
kfree(fw_work);
}
@@ -1319,7 +1320,9 @@ request_firmware_nowait(
return -ENOMEM;
fw_work->module = module;
- fw_work->name = name;
+ fw_work->name = kstrdup_const(name, gfp);
+ if (!fw_work->name)
+ return -ENOMEM;
fw_work->device = device;
fw_work->context = context;
fw_work->cont = cont;
@@ -1327,6 +1330,7 @@ request_firmware_nowait(
(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
if (!try_module_get(module)) {
+ kfree_const(fw_work->name);
kfree(fw_work);
return -EFAULT;
}
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
We currently use flexible arrays with a char at the
end for the remaining internal firmware name uses.
There are two limitations with the way we use this.
Since we're using a flexible array for a string on the
struct if we wanted to use two strings it means we'd
have a disjoint means of handling the strings, one
using the flexible array, and another a char * pointer.
We're also currently not using 'const' for the string.
We wish to later extend some firmware data structures
with other string/char pointers, but we also want to be
very pedantic about const usage. Since we're going to
change things to use 'const' we might as well also address
unified way to use multiple strings on the structs.
Replace the flexible array practice for strings with
kstrdup_const() and kfree_const(), this will avoid
allocations when the vmlinux .rodata is used, and just
allocate a new proper string for us when needed. This
also means we can simplify the struct allocations by
removing the string length from the allocation size
computation, which would otherwise get even more
complicated when supporting multiple strings.
Cc: Linus Torvalds <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: David Howells <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Seth Forshee <[email protected]>
Cc: Kyle McMartin <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ae1ed56..cf1d94e 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -164,17 +164,17 @@ struct firmware_buf {
int page_array_size;
struct list_head pending_list;
#endif
- char fw_id[];
+ const char *fw_id;
};
struct fw_cache_entry {
struct list_head list;
- char name[];
+ const char *name;
};
struct fw_name_devm {
unsigned long magic;
- char name[];
+ const char *name;
};
#define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
@@ -195,13 +195,17 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
{
struct firmware_buf *buf;
- buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_ATOMIC);
-
+ buf = kzalloc(sizeof(*buf), GFP_ATOMIC);
if (!buf)
- return buf;
+ return NULL;
+
+ buf->fw_id = kstrdup_const(fw_name, GFP_ATOMIC);
+ if (!buf->fw_id) {
+ kfree(buf);
+ return NULL;
+ }
kref_init(&buf->ref);
- strcpy(buf->fw_id, fw_name);
buf->fwc = fwc;
init_completion(&buf->completion);
#ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -271,6 +275,7 @@ static void __fw_free_buf(struct kref *ref)
} else
#endif
vfree(buf->data);
+ kfree_const(buf->fw_id);
kfree(buf);
}
@@ -415,6 +420,7 @@ static void fw_name_devm_release(struct device *dev, void *res)
if (fwn->magic == (unsigned long)&fw_cache)
pr_debug("%s: fw_name-%s devm-%p released\n",
__func__, fwn->name, res);
+ kfree_const(fwn->name);
}
static int fw_devm_match(struct device *dev, void *res,
@@ -445,13 +451,17 @@ static int fw_add_devm_name(struct device *dev, const char *name)
if (fwn)
return 1;
- fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm) +
- strlen(name) + 1, GFP_KERNEL);
+ fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
+ GFP_KERNEL);
if (!fwn)
return -ENOMEM;
+ fwn->name = kstrdup_const(name, GFP_KERNEL);
+ if (!fwn->name) {
+ kfree(fwn);
+ return -ENOMEM;
+ }
fwn->magic = (unsigned long)&fw_cache;
- strcpy(fwn->name, name);
devres_add(dev, fwn);
return 0;
@@ -1421,11 +1431,16 @@ static struct fw_cache_entry *alloc_fw_cache_entry(const char *name)
{
struct fw_cache_entry *fce;
- fce = kzalloc(sizeof(*fce) + strlen(name) + 1, GFP_ATOMIC);
+ fce = kzalloc(sizeof(*fce), GFP_ATOMIC);
if (!fce)
goto exit;
- strcpy(fce->name, name);
+ fce->name = kstrdup_const(name, GFP_ATOMIC);
+ if (!fce->name) {
+ kfree(fce);
+ fce = NULL;
+ goto exit;
+ }
exit:
return fce;
}
@@ -1465,6 +1480,7 @@ found:
static void free_fw_cache_entry(struct fw_cache_entry *fce)
{
+ kfree_const(fce->name);
kfree(fce);
}
--
2.3.2.209.gd67f9d5.dirty
On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez
<[email protected]> wrote:
> +
> + path = __getname();
> + if (unlikely(!path))
> + return PTR_ERR(path);
This makes no sense.
PTR_ERR() on NULL is an insane operation. It's a very non-intuitive
and misleading way of writing "0".
So not only is that "return 0;", that's not likely what you want _anyway_.
If you intended to return an error, you should just have done so, eg
if (unlikely(!path))
return -ENOMEM;
which actually does something sane, and is more readable.
PTR_ERR() is for when you get an error pointer, so a sequence like
if (IS_ERR(ptr))
return PTR_ERR(ptr);
is sensible (it checks whether the ptr has an error value in it, and
then returns the integer error value of the pointer).
But for a NULL pointer? No.
Linus
On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez
<[email protected]> wrote:
>
> Instead of waiting until the last second to fail
> on a request_firmware*() calls due to filename
> truncation we can do an early check upon boot
> and immediatley avoid any possible issues upfront.
Why? This looks stupid. Why add this special case, when normal path
lookup results in the proper errors?
And if invalid pathnames are so frequent that this special case is
somehow worth it, we should fix whoever generates that crap, instead
of adding this insane special case.
And if we don't handle the errors from normal path lookup properly,
then we should fix *that*.
In other words, I see absolutely no reason for this patch. Regardless
of the reason for it, it seems entirely broken.
Linus
On Tue, May 12, 2015 at 01:31:58PM -0700, Linus Torvalds wrote:
> On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez
> <[email protected]> wrote:
> > +
> > + path = __getname();
> > + if (unlikely(!path))
> > + return PTR_ERR(path);
>
> This makes no sense.
>
> PTR_ERR() on NULL is an insane operation. It's a very non-intuitive
> and misleading way of writing "0".
>
> So not only is that "return 0;", that's not likely what you want _anyway_.
>
> If you intended to return an error, you should just have done so, eg
>
> if (unlikely(!path))
> return -ENOMEM;
>
> which actually does something sane, and is more readable.
Will fix, thanks.
Luis
On Tue, May 12, 2015 at 01:35:59PM -0700, Linus Torvalds wrote:
> On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez
> <[email protected]> wrote:
> >
> > Instead of waiting until the last second to fail
> > on a request_firmware*() calls due to filename
> > truncation we can do an early check upon boot
> > and immediatley avoid any possible issues upfront.
>
> Why? This looks stupid. Why add this special case, when normal path
> lookup results in the proper errors
It seemed silly to proceed late if we can catch the possible name errors
early. It does indeed have the cost of all that early cruft code.
> And if invalid pathnames are so frequent that this special case is
> somehow worth it, we should fix whoever generates that crap, instead
> of adding this insane special case.
OK, I'm all for ignoring non-upstream drivers.
> And if we don't handle the errors from normal path lookup properly,
> then we should fix *that*.
That was done on patch 2, originally I was going for a simple early
check which can be put on all API calls prior to doing anything too
intrusive:
+static int sysdata_validate_filename(const char *name)
+{
+ if (!name)
+ return -EINVAL;
+ /* POSIX.1 2.4: an empty pathname is invalid, match other checks */
+ if (name[0] == '\0')
+ return -ENOENT;
+}
Since the truncation was possible too though it seemed worthy to add given
that quite a few callers can end up re-using the same code.
> In other words, I see absolutely no reason for this patch. Regardless
> of the reason for it, it seems entirely broken.
OK I'll get rid of all these early checks.
Luis
On Tue, May 12, 2015 at 11:30:53AM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> The request_firmware*() APIs uses __getname() to iterate
> over the list of paths possible for firmware to be found,
> the code however never checked for failure on __getname().
> Although *very unlikely*, this can still happen. Add the
> missing check.
>
> There is still no checks on the concatenation of the path
> and filename passed, that requires a bit more work and
> subsequent patches address this. The commit that introduced
> this is abb139e7 ("firmware: teach the kernel to load
> firmware files directly from the filesystem").
>
> mcgrof@ergon ~/linux (git::firmware-fixes) $ git describe --contains abb139e7
> v3.7-rc1~120
>
> Cc: Linus Torvalds <[email protected]>
> Cc: Ming Lei <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Kyle McMartin <[email protected]>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> drivers/base/firmware_class.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 171841a..bc6c8e6 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device,
> {
> int i;
> int rc = -ENOENT;
> - char *path = __getname();
> + char *path;
> +
> + path = __getname();
> + if (unlikely(!path))
Please only use likely/unlikely on code paths that actually care about
it (i.e. you can measure the difference). Otherwise it is pretty
useless, and people have determined that sometimes it is slower as
humans get this wrong a lot of time...
thanks,
greg k-h
On Tue, May 12, 2015 at 02:21:11PM -0700, Greg KH wrote:
> On Tue, May 12, 2015 at 11:30:53AM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <[email protected]>
> >
> > The request_firmware*() APIs uses __getname() to iterate
> > over the list of paths possible for firmware to be found,
> > the code however never checked for failure on __getname().
> > Although *very unlikely*, this can still happen. Add the
> > missing check.
> >
> > There is still no checks on the concatenation of the path
> > and filename passed, that requires a bit more work and
> > subsequent patches address this. The commit that introduced
> > this is abb139e7 ("firmware: teach the kernel to load
> > firmware files directly from the filesystem").
> >
> > mcgrof@ergon ~/linux (git::firmware-fixes) $ git describe --contains abb139e7
> > v3.7-rc1~120
> >
> > Cc: Linus Torvalds <[email protected]>
> > Cc: Ming Lei <[email protected]>
> > Cc: Rusty Russell <[email protected]>
> > Cc: David Howells <[email protected]>
> > Cc: Kyle McMartin <[email protected]>
> > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > ---
> > drivers/base/firmware_class.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 171841a..bc6c8e6 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device,
> > {
> > int i;
> > int rc = -ENOENT;
> > - char *path = __getname();
> > + char *path;
> > +
> > + path = __getname();
> > + if (unlikely(!path))
>
> Please only use likely/unlikely on code paths that actually care about
> it (i.e. you can measure the difference). Otherwise it is pretty
> useless, and people have determined that sometimes it is slower as
> humans get this wrong a lot of time...
Will do , thanks.
Luis