copy_mount_options()

copy_mount_options()

[David S. Miller]

So the sparc64 user copy bug I fixed recently is pretty
much present on every platform.

Basically, copy_mount_options() requires exact byte granularity
to exception reporting from copy_from_user().  If you don't
do this it can break things like busybox's mount().

Even reporting on a word boundary is illegal.  On sparc64
it was quite poignant because we can report on a 64-byte
boundary for large copies because that is the granularity
of the load/store we use.

Other platforms will need to fix this.  I recommend a two
stage exception handling scheme.  Basically, on the first
exception, you merely note that an exception occurred
and you retry the user copy a byte at a time until you
hit the exact address that fails.  You cannot optimize
this to just check a page at a time, because copy_mount_options
wants all the data to be there.

Re: copy_mount_options()

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> 
> So the sparc64 user copy bug I fixed recently is pretty
> much present on every platform.
> 
> Basically, copy_mount_options() requires exact byte granularity
> to exception reporting from copy_from_user().  If you don't
> do this it can break things like busybox's mount().
> 
> Even reporting on a word boundary is illegal.  On sparc64
> it was quite poignant because we can report on a 64-byte
> boundary for large copies because that is the granularity
> of the load/store we use.
> 
> Other platforms will need to fix this.  I recommend a two
> stage exception handling scheme.  Basically, on the first
> exception, you merely note that an exception occurred
> and you retry the user copy a byte at a time until you
> hit the exact address that fails.  You cannot optimize
> this to just check a page at a time, because copy_mount_options
> wants all the data to be there.

For some reason, copy_mount_options() is a continual pain in the ass.  It
just comes up again and again.

For this problem I'd suggest we just rip the copy_from_user() out of there
and rewrite the function to use byte-at-a-time get_user()s.

Re: copy_mount_options()

[David S. Miller]

On Fri, 20 Aug 2004 13:10:53 -0700
Andrew Morton <akpm@osdl.org> wrote:

> For some reason, copy_mount_options() is a continual pain in the ass.  It
> just comes up again and again.
> 
> For this problem I'd suggest we just rip the copy_from_user() out of there
> and rewrite the function to use byte-at-a-time get_user()s.

I totally agree.

Re: copy_mount_options()

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> On Fri, 20 Aug 2004 13:10:53 -0700
> Andrew Morton <akpm@osdl.org> wrote:
> 
> > For some reason, copy_mount_options() is a continual pain in the ass.  It
> > just comes up again and again.
> > 
> > For this problem I'd suggest we just rip the copy_from_user() out of there
> > and rewrite the function to use byte-at-a-time get_user()s.
> 
> I totally agree.

Something like this?

--- 25/fs/namespace.c~copy_mount_options-size-fix	Fri Aug 20 14:25:39 2004
+++ 25-akpm/fs/namespace.c	Fri Aug 20 14:30:09 2004
@@ -930,7 +930,29 @@ void mark_mounts_for_expiry(struct list_
 
 EXPORT_SYMBOL_GPL(mark_mounts_for_expiry);
 
-int copy_mount_options (const void __user *data, unsigned long *where)
+/*
+ * Some copy_from_user() implementations do not return the exact number of
+ * bytes remaining to copy on a fault.  But copy_mount_options() requires that.
+ */
+
+static long
+exact_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	char *t = to;
+	const char __user *f = from;
+	char c;
+
+	while (n) {
+		if (get_user(c, f))
+			break;
+		*t++ = c;
+		f++;
+		n--;
+	}
+	return n;
+}
+
+int copy_mount_options(const void __user *data, unsigned long *where)
 {
 	int i;
 	unsigned long page;
@@ -952,7 +974,7 @@ int copy_mount_options (const void __use
 	if (size > PAGE_SIZE)
 		size = PAGE_SIZE;
 
-	i = size - copy_from_user((void *)page, data, size);
+	i = size - exact_copy_from_user((void *)page, data, size);
 	if (!i) {
 		free_page(page); 
 		return -EFAULT;
_

Re: copy_mount_options()

[David S. Miller]

On Fri, 20 Aug 2004 14:31:11 -0700
Andrew Morton <akpm@osdl.org> wrote:

> Something like this?

Looks pretty good.  Although I would do a verify_area() above
the loop and then use __get_user().

I know this doesn't need to be fast, but it doesn't need to
be that slow either :-)))

Re: copy_mount_options()

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> > Something like this?
> 
> Looks pretty good.  Although I would do a verify_area() above
> the loop and then use __get_user().

OK, here's #2, with enhanced comment and changelog!

Untested though.



davem says that copy_mount_options is failing in obscure ways if the
architecture's copy_from_user() doesn't return an exact count of the number of
uncopied bytes.

Fixing that up in each architecture is a pain - it involves falling back to
byte-at-a-time copies.

It's simple to open-code this in namespace.c.  If we find other places in the
kernel which care about this we can promote this to a global function.

Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/fs/namespace.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff -puN fs/namespace.c~copy_mount_options-size-fix fs/namespace.c
--- 25/fs/namespace.c~copy_mount_options-size-fix	Fri Aug 20 15:43:26 2004
+++ 25-akpm/fs/namespace.c	Fri Aug 20 15:46:05 2004
@@ -930,7 +930,33 @@ void mark_mounts_for_expiry(struct list_
 
 EXPORT_SYMBOL_GPL(mark_mounts_for_expiry);
 
-int copy_mount_options (const void __user *data, unsigned long *where)
+/*
+ * Some copy_from_user() implementations do not return the exact number of
+ * bytes remaining to copy on a fault.  But copy_mount_options() requires that.
+ * Note that this function differs from copy_from_user() in that it will oops
+ * on bad values of `to', rather than returning a short copy.
+ */
+static long
+exact_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	char *t = to;
+	const char __user *f = from;
+	char c;
+
+	if (!access_ok(VERIFY_READ, from, n))
+		return n;
+
+	while (n) {
+		if (__get_user(c, f))
+			break;
+		*t++ = c;
+		f++;
+		n--;
+	}
+	return n;
+}
+
+int copy_mount_options(const void __user *data, unsigned long *where)
 {
 	int i;
 	unsigned long page;
@@ -952,7 +978,7 @@ int copy_mount_options (const void __use
 	if (size > PAGE_SIZE)
 		size = PAGE_SIZE;
 
-	i = size - copy_from_user((void *)page, data, size);
+	i = size - exact_copy_from_user((void *)page, data, size);
 	if (!i) {
 		free_page(page); 
 		return -EFAULT;
_

Re: copy_mount_options()

[Anton Blanchard]

> OK, here's #2, with enhanced comment and changelog!
> 
> Untested though.

That removes one more place that uses the return value of 
copy_to/from_user.

Queue Rusty's "copy_from_user is a deathtrap" spiel:

http://www.ussg.iu.edu/hypermail/linux/kernel/0205.2/0193.html

I too hate that interface, Im continually getting it wrong. It would be
nice to remove the possibility of having similar such subtle bugs.

Anton

> davem says that copy_mount_options is failing in obscure ways if the
> architecture's copy_from_user() doesn't return an exact count of the number of
> uncopied bytes.
> 
> Fixing that up in each architecture is a pain - it involves falling back to
> byte-at-a-time copies.
> 
> It's simple to open-code this in namespace.c.  If we find other places in the
> kernel which care about this we can promote this to a global function.
> 
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
> 
>  25-akpm/fs/namespace.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff -puN fs/namespace.c~copy_mount_options-size-fix fs/namespace.c
> --- 25/fs/namespace.c~copy_mount_options-size-fix	Fri Aug 20 15:43:26 2004
> +++ 25-akpm/fs/namespace.c	Fri Aug 20 15:46:05 2004
> @@ -930,7 +930,33 @@ void mark_mounts_for_expiry(struct list_
>  
>  EXPORT_SYMBOL_GPL(mark_mounts_for_expiry);
>  
> -int copy_mount_options (const void __user *data, unsigned long *where)
> +/*
> + * Some copy_from_user() implementations do not return the exact number of
> + * bytes remaining to copy on a fault.  But copy_mount_options() requires that.
> + * Note that this function differs from copy_from_user() in that it will oops
> + * on bad values of `to', rather than returning a short copy.
> + */
> +static long
> +exact_copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> +	char *t = to;
> +	const char __user *f = from;
> +	char c;
> +
> +	if (!access_ok(VERIFY_READ, from, n))
> +		return n;
> +
> +	while (n) {
> +		if (__get_user(c, f))
> +			break;
> +		*t++ = c;
> +		f++;
> +		n--;
> +	}
> +	return n;
> +}
> +
> +int copy_mount_options(const void __user *data, unsigned long *where)
>  {
>  	int i;
>  	unsigned long page;
> @@ -952,7 +978,7 @@ int copy_mount_options (const void __use
>  	if (size > PAGE_SIZE)
>  		size = PAGE_SIZE;
>  
> -	i = size - copy_from_user((void *)page, data, size);
> +	i = size - exact_copy_from_user((void *)page, data, size);
>  	if (!i) {
>  		free_page(page); 
>  		return -EFAULT;
> _

Re: copy_mount_options()

[David S. Miller]

On Sat, 21 Aug 2004 09:18:33 +1000
Anton Blanchard <anton@samba.org> wrote:

> I too hate that interface, Im continually getting it wrong. It would be
> nice to remove the possibility of having similar such subtle bugs.

I agree.

There used to be clever code that would truncate the pipe write
and stuff like that, and TCP even used to do something similar
at one point.  But I go look now and neither of them do that
any more.

It would even make the kernel smaller because all of these silly:

	return copy_*_user(...) ? -EFAULT : 0;

would just expand to a direct use of the return value.

So many arch user copy routines would have enormous amounts
of complexity removed, and I saw this quite well when doing
the sparc64 stuff I did yesterday.

Re: copy_mount_options()

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> On Sat, 21 Aug 2004 09:18:33 +1000
> Anton Blanchard <anton@samba.org> wrote:
> 
> > I too hate that interface, Im continually getting it wrong. It would be
> > nice to remove the possibility of having similar such subtle bugs.
> 
> I agree.
> 
> There used to be clever code that would truncate the pipe write
> and stuff like that, and TCP even used to do something similar
> at one point.  But I go look now and neither of them do that
> any more.
> 
> It would even make the kernel smaller because all of these silly:
> 
> 	return copy_*_user(...) ? -EFAULT : 0;
> 
> would just expand to a direct use of the return value.
> 
> So many arch user copy routines would have enormous amounts
> of complexity removed, and I saw this quite well when doing
> the sparc64 stuff I did yesterday.

I'm all for it.  I'll sneak the below patch into -mm, see what breaks.

It's a bit weird that the usercopy functions return unsigned long.  If they
are to return -EFAULT they really should be changed to return an int.

--- 25/arch/i386/lib/usercopy.c~usercopy-return-EFAULT	Fri Aug 20 17:04:37 2004
+++ 25-akpm/arch/i386/lib/usercopy.c	Fri Aug 20 17:05:52 2004
@@ -560,14 +560,14 @@ survive:
 			to += len;
 			n -= len;
 		}
-		return n;
+		return n ? -EFAULT : 0;
 	}
 #endif
 	if (movsl_is_ok(to, from, n))
 		__copy_user(to, from, n);
 	else
 		n = __copy_user_intel(to, from, n);
-	return n;
+	return n ? -EFAULT : 0;
 }
 
 unsigned long
@@ -577,7 +577,7 @@ __copy_from_user_ll(void *to, const void
 		__copy_user_zeroing(to, from, n);
 	else
 		n = __copy_user_zeroing_intel(to, from, n);
-	return n;
+	return n ? -EFAULT : 0;
 }
 
 /**
@@ -599,7 +599,7 @@ copy_to_user(void __user *to, const void
 	might_sleep();
 	if (access_ok(VERIFY_WRITE, to, n))
 		n = __copy_to_user(to, from, n);
-	return n;
+	return n ? -EFAULT : 0;
 }
 EXPORT_SYMBOL(copy_to_user);
 
@@ -627,6 +627,6 @@ copy_from_user(void *to, const void __us
 		n = __copy_from_user(to, from, n);
 	else
 		memset(to, 0, n);
-	return n;
+	return n ? -EFAULT : 0;
 }
 EXPORT_SYMBOL(copy_from_user);
_

Re: copy_mount_options()

[Rusty Russell]

On Sat, 2004-08-21 at 10:07, Andrew Morton wrote:
> I'm all for it.  I'll sneak the below patch into -mm, see what breaks.

You should zero out the whole buffer in copy_from_user() if you return
-EFAULT.  We had bugs where people didn't check the returns and you
could read random junk.

As to who uses the value, generic file read and write still use it, as
did some of the serial code last I checked.  Linus was of the belief
that they should do a short read/write up to the fault boundary rather
than return -EFAULT.

There's evidence that noone relies on such behaviour, though, since at
the time of the debate, doing such a thing would cause an OOPS on ppc. 
Also, this behaviour silently changed after 2.0 and noone noticed. 
Finally, other unixes are varied in their approaches in this case (some
downright buggy, lying about how much they'd written).

I would humbly suggest an additional option which sent a SEGV to the
process, as well.  If you were playing mprotect games you'd expect them,
and if you weren't, you're probably buggy.  After a while, we can simply
forget about checking returns from copy_to/from_user, which would be a
blissful simplification of kernel code.

Cheers,
Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: copy_mount_options()

[Benjamin Herrenschmidt]

On Sat, 2004-08-21 at 06:01, David S. Miller wrote:
> So the sparc64 user copy bug I fixed recently is pretty
> much present on every platform.
> 
> Basically, copy_mount_options() requires exact byte granularity
> to exception reporting from copy_from_user().  If you don't
> do this it can break things like busybox's mount().

Yup, old problem, reported before several times. Another issue
that pops up with it and busybox is that copy_mount_options will
touch an entire page from the passed pointer. If the pointer you
pass is near the end of your data/bss area, you end up touching
the area between stack and bss, and potentially cause the stack
to grow all the way down to the bss.

We have saveguards against that now on ppc, I suppose x86 has too,
but it may be worth reminding other archs to have a look.

> Even reporting on a word boundary is illegal.  On sparc64
> it was quite poignant because we can report on a 64-byte
> boundary for large copies because that is the granularity
> of the load/store we use.
> 
> Other platforms will need to fix this.  I recommend a two
> stage exception handling scheme.  Basically, on the first
> exception, you merely note that an exception occurred
> and you retry the user copy a byte at a time until you
> hit the exact address that fails.  You cannot optimize
> this to just check a page at a time, because copy_mount_options
> wants all the data to be there.
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: copy_mount_options()

[Ralf Baechle]

On Sat, Aug 21, 2004 at 09:15:46AM +1000, Benjamin Herrenschmidt wrote:

> > Basically, copy_mount_options() requires exact byte granularity
> > to exception reporting from copy_from_user().  If you don't
> > do this it can break things like busybox's mount().
> 
> Yup, old problem, reported before several times. Another issue
> that pops up with it and busybox is that copy_mount_options will
> touch an entire page from the passed pointer. If the pointer you
> pass is near the end of your data/bss area, you end up touching
> the area between stack and bss, and potentially cause the stack
> to grow all the way down to the bss.

On MIPS it's been triggering a special case in verify_area which make
it return -EFAULT if the arguments area includes the last byte of the
2GB userspace as busybox does.  A proper fix of verify_area would have
inflated the verify_area() code by one instruction so I reduced
TASK_SIZE a bit ...

  Ralf