* [3.10-rc1 PATCH] devtmpfs: Fix kmemcheck warning.
@ 2013-05-14 12:02 Tetsuo Handa
2013-08-22 17:32 ` Dave Hansen
0 siblings, 1 reply; 2+ messages in thread
From: Tetsuo Handa @ 2013-05-14 12:02 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel
I got below warning.
WARNING: kmemcheck: Caught 8-bit read from uninitialized memory (ffff88007ae384d8)
00000000000000000000000000000000d884e37a0088ffff006f665f64657669
i i i i i i i i i i i i i i i i i i i i i i i i u u u u u u u u
^
RIP: 0010:[<ffffffff81169c2d>] [<ffffffff81169c2d>] copy_mount_options+0xfd/0x1b0
RSP: 0000:ffff88007ae37d68 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff88007ae37da0 RCX: 00000000000000ff
RDX: ffff88007ae384d8 RSI: 0000000000000000 RDI: ffff88007ad776e0
RBP: ffff88007ae37d88 R08: 0000000000000000 R09: ffffffff81ca0130
R10: 000000000007f000 R11: 0000000000080000 R12: 0000000000000920
R13: 0000000000001000 R14: ffff88007ad77000 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff88007b200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff88007ac28404 CR3: 0000000001c0b000 CR4: 00000000000407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
[<ffffffff8116d31d>] SyS_mount+0x6d/0xe0
[<ffffffff813bddd2>] devtmpfsd+0x62/0x170
[<ffffffff81065f3e>] kthread+0xee/0x100
[<ffffffff817a746c>] ret_from_fork+0x7c/0xb0
[<ffffffffffffffff>] 0xffffffffffffffff
Below patch fixes this warning, but is simpler fix
- char options[] = "mode=0755";
+ static char options[PAGE_SIZE] = "mode=0755";
better?
--------------------
>From 4e768f2e7ea75786a69baae52469e1662244d933 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 14 May 2013 16:32:05 +0900
Subject: [PATCH] devfs: Fix kmemcheck warning.
The "void __user *data" argument passed to mount() has to be PAGE_SIZE bytes of
initialized memory region.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/base/devtmpfs.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 7413d06..59a2baf 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -375,12 +375,24 @@ static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
static int devtmpfsd(void *p)
{
- char options[] = "mode=0755";
+ char *options;
int *err = p;
*err = sys_unshare(CLONE_NEWNS);
if (*err)
goto out;
+ /*
+ * The options argument has to be PAGE_SIZE bytes of initialized memory
+ * region, or kmemcheck will complain "read from uninitialized memory"
+ * because copy_mount_options() tries to copy PAGE_SIZE bytes.
+ */
+ options = (char *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
+ if (!options) {
+ *err = -ENOMEM;
+ goto out;
+ }
+ strcpy(options, "mode=0755");
*err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
+ free_page((unsigned long) options);
if (*err)
goto out;
sys_chdir("/.."); /* will traverse into overmounted root */
--
1.7.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [3.10-rc1 PATCH] devtmpfs: Fix kmemcheck warning.
2013-05-14 12:02 [3.10-rc1 PATCH] devtmpfs: Fix kmemcheck warning Tetsuo Handa
@ 2013-08-22 17:32 ` Dave Hansen
0 siblings, 0 replies; 2+ messages in thread
From: Dave Hansen @ 2013-08-22 17:32 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-fsdevel, linux-kernel, kay@vrfy.org, Al Viro
cc'ing Al and Kay who have the most commits in devtmpfs...
On 05/14/2013 05:02 AM, Tetsuo Handa wrote:
> I got below warning.
>
> WARNING: kmemcheck: Caught 8-bit read from uninitialized memory (ffff88007ae384d8)
> 00000000000000000000000000000000d884e37a0088ffff006f665f64657669
> i i i i i i i i i i i i i i i i i i i i i i i i u u u u u u u u
> ^
> RIP: 0010:[<ffffffff81169c2d>] [<ffffffff81169c2d>] copy_mount_options+0xfd/0x1b0
> RSP: 0000:ffff88007ae37d68 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffff88007ae37da0 RCX: 00000000000000ff
> RDX: ffff88007ae384d8 RSI: 0000000000000000 RDI: ffff88007ad776e0
> RBP: ffff88007ae37d88 R08: 0000000000000000 R09: ffffffff81ca0130
> R10: 000000000007f000 R11: 0000000000080000 R12: 0000000000000920
> R13: 0000000000001000 R14: ffff88007ad77000 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff88007b200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffff88007ac28404 CR3: 0000000001c0b000 CR4: 00000000000407f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
> [<ffffffff8116d31d>] SyS_mount+0x6d/0xe0
> [<ffffffff813bddd2>] devtmpfsd+0x62/0x170
> [<ffffffff81065f3e>] kthread+0xee/0x100
> [<ffffffff817a746c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> Below patch fixes this warning, but is simpler fix
>
> - char options[] = "mode=0755";
> + static char options[PAGE_SIZE] = "mode=0755";
Yeah, that'll eat up 4k of data space which is a bit silly for this.
> better?
> --------------------
>>From 4e768f2e7ea75786a69baae52469e1662244d933 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Wed, 14 May 2013 16:32:05 +0900
> Subject: [PATCH] devfs: Fix kmemcheck warning.
>
> The "void __user *data" argument passed to mount() has to be PAGE_SIZE bytes of
> initialized memory region.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> drivers/base/devtmpfs.c | 14 +++++++++++++-
> 1 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 7413d06..59a2baf 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -375,12 +375,24 @@ static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
>
> static int devtmpfsd(void *p)
> {
> - char options[] = "mode=0755";
> + char *options;
> int *err = p;
> *err = sys_unshare(CLONE_NEWNS);
> if (*err)
> goto out;
> + /*
> + * The options argument has to be PAGE_SIZE bytes of initialized memory
> + * region, or kmemcheck will complain "read from uninitialized memory"
> + * because copy_mount_options() tries to copy PAGE_SIZE bytes.
> + */
> + options = (char *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
> + if (!options) {
> + *err = -ENOMEM;
> + goto out;
> + }
> + strcpy(options, "mode=0755");
> *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
> + free_page((unsigned long) options);
> if (*err)
> goto out;
> sys_chdir("/.."); /* will traverse into overmounted root */
I do not think it is a false positive. devtmpfsd is passing a
stack-allocated value in to copy_mount_options() which does a
exact_copy_from_user() of what appears to usually be a PAGE_SIZE chunk
of data.
Most of the time, it's probably just innocently reading some kernel
stack data. It doesn't hurt anything on x86 since we are quite shallow
in the call stack, and we have 2 pages worth of stack space. But, if
that call stack went deeper, or in places where we only have PAGE_SIZE
for a kernel stack, it could potentially go off and read memory it
shouldn't be.
That's not going to be harmful very often, but I do think there can be
side-effects if we happened to do reads from I/O space that we didn't
mean to. I'm also not sure that it's generally wise to be doing
__get_user() on possibly non-present *kernel* memory, but I can't think
of a scenario where it's actively harmful.
As for the patch... exact_copy_from_user() doesn't require or enforce
any alignment constraints, so a simple kmalloc() might be a better idea
here. It'll at least save you an ugly cast or two.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-08-22 17:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-14 12:02 [3.10-rc1 PATCH] devtmpfs: Fix kmemcheck warning Tetsuo Handa
2013-08-22 17:32 ` Dave Hansen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.