* [patch 07/21] fs: fix overflow in sys_mount() for in-kernel calls
@ 2009-09-18 20:05 akpm
2009-09-21 10:34 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: akpm @ 2009-09-18 20:05 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, akpm, vegard.nossum, mingo, penberg
From: Vegard Nossum <vegard.nossum@gmail.com>
On 2009/6/17 Ingo Molnar <mingo@elte.hu> reported:
>
> btw., here's an old friend of a warning:
>
> async_continuing @ 1 after 0 usec
> WARNING: kmemcheck: Caught 8-bit read from freed memory (f5f33004)
> 0040f3f57400686f74706c756700000000000000000000000000000000000000
> i i i i f f f f f f f f f f f f f f f f f f f f f f f f f f f f
> ^
>
> Pid: 1, comm: swapper Not tainted (2.6.30-tip-04303-g5ada65e-dirty #767) P4DC6
> EIP: 0060:[<c1248df4>] EFLAGS: 00010246 CPU: 0
> EIP is at exact_copy_from_user+0x64/0x130
> EAX: 00000000 EBX: 00000001 ECX: 000000f5 EDX: 000000f5
> ESI: f5fdeffb EDI: f5f33004 EBP: f6c48ee8 ESP: c29598cc
> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> CR0: 8005003b CR2: f6c20044 CR3: 0294d000 CR4: 000006d0
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff4ff0 DR7: 00000400
> [<c124916a>] copy_mount_options+0xba/0x1c0
> [<c124dc0a>] sys_mount+0x1a/0x170
> [<c263c937>] do_mount_root+0x27/0xe0
> [<c263ca33>] mount_block_root+0x43/0x140
> [<c263cc02>] mount_root+0xd2/0x160
> [<c263ce49>] prepare_namespace+0x1b9/0x380
> [<c263c4c8>] kernel_init+0xb8/0x110
> [<c103ab13>] kernel_thread_helper+0x7/0x14
> [<ffffffff>] 0xffffffff
> EXT3-fs: INFO: recovery required on readonly filesystem.
> EXT3-fs: write access will be enabled during recovery.
sys_mount() reads/copies a whole page for its "type" parameter. When
do_mount_root() passes a kernel address that points to an object which is
smaller than a whole page, copy_mount_options() will happily go past this
memory object, possibly dereferencing "wild" pointers that could be in any
state (hence the kmemcheck warning, which shows that parts of the next
page are not even allocated).
(The likelihood of something going wrong here is pretty low -- first of
all this only applies to kernel calls to sys_mount(), which are mostly
found in the boot code. Secondly, I guess if the page was not mapped,
exact_copy_from_user() _would_ in fact handle it correctly because of its
access_ok(), etc. checks.)
But it is much nicer to avoid the dubious reads altogether, by stopping as
soon as we find a NUL byte. Is there a good reason why we can't do
something like this, using the already existing strndup_from_user()?
[akpm@linux-foundation.org: make copy_mount_string() static]
Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
fs/namespace.c | 81 ++++++++++++++++++++++++++++-------------------
1 file changed, 49 insertions(+), 32 deletions(-)
diff -puN fs/namespace.c~fs-fix-overflow-in-sys_mount-for-in-kernel-calls fs/namespace.c
--- a/fs/namespace.c~fs-fix-overflow-in-sys_mount-for-in-kernel-calls
+++ a/fs/namespace.c
@@ -1640,7 +1640,7 @@ static int do_new_mount(struct path *pat
{
struct vfsmount *mnt;
- if (!type || !memchr(type, 0, PAGE_SIZE))
+ if (!type)
return -EINVAL;
/* we need capabilities... */
@@ -1871,6 +1871,23 @@ int copy_mount_options(const void __user
return 0;
}
+static int copy_mount_string(const void __user *data, char **where)
+{
+ char *tmp;
+
+ if (!data) {
+ *where = NULL;
+ return 0;
+ }
+
+ tmp = strndup_user(data, PAGE_SIZE);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ *where = tmp;
+ return 0;
+}
+
/*
* Flags is a 32-bit value that allows up to 31 non-fs dependent flags to
* be given to the mount() call (ie: read-only, no-dev, no-suid etc).
@@ -1900,8 +1917,6 @@ long do_mount(char *dev_name, char *dir_
if (!dir_name || !*dir_name || !memchr(dir_name, 0, PAGE_SIZE))
return -EINVAL;
- if (dev_name && !memchr(dev_name, 0, PAGE_SIZE))
- return -EINVAL;
if (data_page)
((char *)data_page)[PAGE_SIZE - 1] = 0;
@@ -2070,40 +2085,42 @@ EXPORT_SYMBOL(create_mnt_ns);
SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
char __user *, type, unsigned long, flags, void __user *, data)
{
- int retval;
+ int ret;
+ char *kernel_type;
+ char *kernel_dir;
+ char *kernel_dev;
unsigned long data_page;
- unsigned long type_page;
- unsigned long dev_page;
- char *dir_page;
- retval = copy_mount_options(type, &type_page);
- if (retval < 0)
- return retval;
+ ret = copy_mount_string(type, &kernel_type);
+ if (ret < 0)
+ goto out_type;
+
+ kernel_dir = getname(dir_name);
+ if (IS_ERR(kernel_dir)) {
+ ret = PTR_ERR(kernel_dir);
+ goto out_dir;
+ }
+
+ ret = copy_mount_string(dev_name, &kernel_dev);
+ if (ret < 0)
+ goto out_dev;
+
+ ret = copy_mount_options(data, &data_page);
+ if (ret < 0)
+ goto out_data;
- dir_page = getname(dir_name);
- retval = PTR_ERR(dir_page);
- if (IS_ERR(dir_page))
- goto out1;
-
- retval = copy_mount_options(dev_name, &dev_page);
- if (retval < 0)
- goto out2;
-
- retval = copy_mount_options(data, &data_page);
- if (retval < 0)
- goto out3;
+ ret = do_mount(kernel_dev, kernel_dir, kernel_type, flags,
+ (void *) data_page);
- retval = do_mount((char *)dev_page, dir_page, (char *)type_page,
- flags, (void *)data_page);
free_page(data_page);
-
-out3:
- free_page(dev_page);
-out2:
- putname(dir_page);
-out1:
- free_page(type_page);
- return retval;
+out_data:
+ kfree(kernel_dev);
+out_dev:
+ putname(kernel_dir);
+out_dir:
+ kfree(kernel_type);
+out_type:
+ return ret;
}
/*
_
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 07/21] fs: fix overflow in sys_mount() for in-kernel calls
2009-09-18 20:05 [patch 07/21] fs: fix overflow in sys_mount() for in-kernel calls akpm
@ 2009-09-21 10:34 ` Al Viro
2009-09-23 6:26 ` Pekka Enberg
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2009-09-21 10:34 UTC (permalink / raw)
To: akpm; +Cc: linux-fsdevel, vegard.nossum, mingo, penberg
On Fri, Sep 18, 2009 at 01:05:45PM -0700, akpm@linux-foundation.org wrote:
> From: Vegard Nossum <vegard.nossum@gmail.com>
>
> On 2009/6/17 Ingo Molnar <mingo@elte.hu> reported:
> >
> > btw., here's an old friend of a warning:
> >
[snip]
> @@ -1640,7 +1640,7 @@ static int do_new_mount(struct path *pat
> {
> struct vfsmount *mnt;
>
> - if (!type || !memchr(type, 0, PAGE_SIZE))
> + if (!type)
> return -EINVAL;
>
> /* we need capabilities... */
> @@ -1871,6 +1871,23 @@ int copy_mount_options(const void __user
> return 0;
> }
> @@ -1900,8 +1917,6 @@ long do_mount(char *dev_name, char *dir_
>
> if (!dir_name || !*dir_name || !memchr(dir_name, 0, PAGE_SIZE))
> return -EINVAL;
> - if (dev_name && !memchr(dev_name, 0, PAGE_SIZE))
> - return -EINVAL;
>
> if (data_page)
> ((char *)data_page)[PAGE_SIZE - 1] = 0;
In this form it's simply b0rken - you need to take care of other do_mount()
callers. arch/alpha/kernel/osf_sys.c is safe, but fs/compat.c isn't...
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 07/21] fs: fix overflow in sys_mount() for in-kernel calls
2009-09-21 10:34 ` Al Viro
@ 2009-09-23 6:26 ` Pekka Enberg
0 siblings, 0 replies; 3+ messages in thread
From: Pekka Enberg @ 2009-09-23 6:26 UTC (permalink / raw)
To: Al Viro; +Cc: akpm, linux-fsdevel, vegard.nossum, mingo
On Mon, 2009-09-21 at 11:34 +0100, Al Viro wrote:
> On Fri, Sep 18, 2009 at 01:05:45PM -0700, akpm@linux-foundation.org wrote:
> > From: Vegard Nossum <vegard.nossum@gmail.com>
> >
> > On 2009/6/17 Ingo Molnar <mingo@elte.hu> reported:
> > >
> > > btw., here's an old friend of a warning:
> > >
>
> [snip]
>
> > @@ -1640,7 +1640,7 @@ static int do_new_mount(struct path *pat
> > {
> > struct vfsmount *mnt;
> >
> > - if (!type || !memchr(type, 0, PAGE_SIZE))
> > + if (!type)
> > return -EINVAL;
> >
> > /* we need capabilities... */
> > @@ -1871,6 +1871,23 @@ int copy_mount_options(const void __user
> > return 0;
> > }
>
> > @@ -1900,8 +1917,6 @@ long do_mount(char *dev_name, char *dir_
> >
> > if (!dir_name || !*dir_name || !memchr(dir_name, 0, PAGE_SIZE))
> > return -EINVAL;
> > - if (dev_name && !memchr(dev_name, 0, PAGE_SIZE))
> > - return -EINVAL;
> >
> > if (data_page)
> > ((char *)data_page)[PAGE_SIZE - 1] = 0;
>
> In this form it's simply b0rken - you need to take care of other do_mount()
> callers. arch/alpha/kernel/osf_sys.c is safe, but fs/compat.c isn't...
Vegard, ping?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-09-23 6:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-18 20:05 [patch 07/21] fs: fix overflow in sys_mount() for in-kernel calls akpm
2009-09-21 10:34 ` Al Viro
2009-09-23 6:26 ` Pekka Enberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).