[PATCH v3 0/4] firmware: few fixes for name uses

[PATCH v3 0/4] firmware: few fixes for name uses

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This v3 changes patch 1 to just return -ENOMEM and remove the
unlikely() optimization. It also drops the early truncation
checks.

Luis R. Rodriguez (4):
  firmware: fix __getname() missing failure check
  firmware: check for file truncation on direct firmware loading
  firmware: fix possible use after free on name on asynchronous request
  firmware: use const for remaining firmware names

 drivers/base/firmware_class.c | 61 +++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

-- 
2.3.2.209.gd67f9d5.dirty

[PATCH v3 1/4] firmware: fix __getname() missing failure check

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <torvalds@linux-foundation.org>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: David Howells <dhowells@redhat.com>
Cc: Kyle McMartin <kyle@kernel.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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..49139a1 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 (!path)
+		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
-- 
2.3.2.209.gd67f9d5.dirty

[PATCH v3 2/4] firmware: check for file truncation on direct firmware loading

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <torvalds@linux-foundation.org>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: David Howells <dhowells@redhat.com>
Cc: Kyle McMartin <kyle@kernel.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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 49139a1..9ffa707 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

[PATCH v3 3/4] firmware: fix possible use after free on name on asynchronous request

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <torvalds@linux-foundation.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: David Howells <dhowells@redhat.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Kyle McMartin <kyle@kernel.org>
Generated-by: Coccinelle SmPL
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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 9ffa707..62e4611 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1256,6 +1256,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);
 }
 
@@ -1295,7 +1296,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;
@@ -1303,6 +1306,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

[PATCH v3 4/4] firmware: use const for remaining firmware names

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <torvalds@linux-foundation.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: David Howells <dhowells@redhat.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Kyle McMartin <kyle@kernel.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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 62e4611..8c3aa3c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -150,17 +150,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)
@@ -181,13 +181,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
@@ -257,6 +261,7 @@ static void __fw_free_buf(struct kref *ref)
 	} else
 #endif
 		vfree(buf->data);
+	kfree_const(buf->fw_id);
 	kfree(buf);
 }
 
@@ -401,6 +406,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,
@@ -431,13 +437,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;
@@ -1397,11 +1407,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;
 }
@@ -1441,6 +1456,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

Re: [PATCH v3 0/4] firmware: few fixes for name uses

[Luis R. Rodriguez]

On Tue, May 12, 2015 at 02:49:39PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This v3 changes patch 1 to just return -ENOMEM and remove the
> unlikely() optimization. It also drops the early truncation
> checks.
> 
> Luis R. Rodriguez (4):
>   firmware: fix __getname() missing failure check
>   firmware: check for file truncation on direct firmware loading
>   firmware: fix possible use after free on name on asynchronous request
>   firmware: use const for remaining firmware names
> 
>  drivers/base/firmware_class.c | 61 +++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)

Ming, a review would be much appreciated. I also have a few other changes coming
later, so should these go through Greg or Takashi first ? If its of any help
I'm happy to take on co-maintenance on this module given that I am lookng
to do quite a bit of surgery on the APIs later.

 Luis

Re: [PATCH v3 0/4] firmware: few fixes for name uses

[Greg KH]

On Tue, May 19, 2015 at 06:45:33PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 12, 2015 at 02:49:39PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This v3 changes patch 1 to just return -ENOMEM and remove the
> > unlikely() optimization. It also drops the early truncation
> > checks.
> > 
> > Luis R. Rodriguez (4):
> >   firmware: fix __getname() missing failure check
> >   firmware: check for file truncation on direct firmware loading
> >   firmware: fix possible use after free on name on asynchronous request
> >   firmware: use const for remaining firmware names
> > 
> >  drivers/base/firmware_class.c | 61 +++++++++++++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> Ming, a review would be much appreciated. I also have a few other changes coming
> later, so should these go through Greg or Takashi first ? If its of any help
> I'm happy to take on co-maintenance on this module given that I am lookng
> to do quite a bit of surgery on the APIs later.

They should go through me.

thanks,

greg k-h