Re: bug in generic strncpy_from_user

Re: bug in generic strncpy_from_user

[Heiko Carstens]

On Mon, Feb 25, 2013 at 09:24:31PM -0800, Linus Torvalds wrote:
> On Mon, Feb 25, 2013 at 8:54 PM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > FWIW, I think the generic strncpy_from_user() implemention has the same
> > bug as the s390 variant:
> >
> > Following example:
> >
> > If there is a NUL terminated string "a\0" starting at e.g. 0xfffe this
> > will cause an (invalid) page fault for the subsequent page at 0x10000
> > in case of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS for e.g. this invocation:
> >
> > strncpy_from_user(kname, 0xfffe, 4096);
> >
> > If the user space page that contained the string was the last within a
> > VMA this may cause a -EFAULT return value for the __get_user() invocation
> > in strncpy_from_user() and byte-at-a-time processing will happen.
> >
> > This is true and works fine unless the next VMA is the stack.
> >
> > With "ulimit -s unlimited" it will just grow downwards to where the page
> > fault happened instead that -EFAULT will be returned.
> 
> This should _never_ be the case.

I was wrong. -EFAULT will be returned, however the vma will grow nevertheless.
Which in turn will cause trouble.

> Our stack-down growing handling requires the whole guard page, and
> should never grow down to touch the previous vma.
> 
> If it does, that's a bug in our stack growing, not in
> strncpy_from_user(). Which could definitely be the case, of course.
> But we should fix it there, not change the strncpy.

After all expand_stack() simply expands the stack to the requested address,
since the guard page is considered to be not included it will just grow
exactly down to the previous mapping.
Only in handle_mm_fault() afterwards there is a check if the access happened
within the guard page which then will make the access fault.
However the stack vma has already grown and user space is broken afterwards.

Here an example: this is before the kernel accesses memory right behind the
ld.so.cache mapping:

[root@p2345007 ~]# cat /proc/2285/maps
00400000-00401000 r-xp 00000000 5e:01 148858                             /root/a.out
00401000-00402000 rw-p 00000000 5e:01 148858                             /root/a.out
40000000-4001c000 r-xp 00000000 5e:01 260585                             /lib/ld-2.14.1.so
4001c000-4001e000 rw-p 0001b000 5e:01 260585                             /lib/ld-2.14.1.so
4001e000-40020000 r-xp 00000000 00:00 0                                  [vdso]
40020000-40023000 rw-p 00000000 00:00 0
40023000-4002c000 r--p 00000000 5e:01 39                                 /etc/ld.so.cache
7ffdf000-80000000 rw-p 00000000 00:00 0                                  [stack]

Then an "open" system call to the file  "/lib/libc.so.6" happens. The string is
null terminated and completely fits into the 0x4002b000 user space page. However
when copying from user space with strncpy_from_user() the kernel also reads from
the 0x4002c000 page.
This generates a fault and grows the stack vma downwards to 0x4002c000:

[root@p2345007 ~]# cat /proc/2285/maps
00400000-00401000 r-xp 00000000 5e:01 148858                             /root/a.out
00401000-00402000 rw-p 00000000 5e:01 148858                             /root/a.out
40000000-4001c000 r-xp 00000000 5e:01 260585                             /lib/ld-2.14.1.so
4001c000-4001e000 rw-p 0001b000 5e:01 260585                             /lib/ld-2.14.1.so
4001e000-40020000 r-xp 00000000 00:00 0                                  [vdso]
40020000-40023000 rw-p 00000000 00:00 0
40023000-4002c000 r--p 00000000 5e:01 39                                 /etc/ld.so.cache
4002d000-80000000 rw-p 00000000 00:00 0                                  [stack]

/proc/maps explicitly excludes the guard page, so the vma of the stack indeed
starts at 0x4002c000. Only every access to the guard page will still fault.

Re: bug in generic strncpy_from_user

[Linus Torvalds]

On Tue, Feb 26, 2013 at 4:57 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
>
> I was wrong. -EFAULT will be returned, however the vma will grow nevertheless.
> Which in turn will cause trouble.

Ok. We should fix that too.

There whole "access just past the end of the previous vma" should
never cause the stack above to expand. The guard page at least gives
people a SIGSEGV, but one of the main reasons for the guard page was
actually to make sure that new "mmap()" calls do not create mappings
just under the stack (in addition to the obvious SIGSEGV when you then
access into that thing).

So while part of the meaning of the guard page is to get that SIGSEGV,
that part is "for safetly". And apparently it works. But at the same
time, there is absolutely no reason to ever expand the stack only to
hit the guard page _anyway_, so if the stack expansion will cause the
requested address to be in the guard page, then the stack expansion
should just have failed.

I think the problem is that we add the guard page *after* we do the
normal "let's try to expand" logic.

I'll take a look.

     Linus

Re: bug in generic strncpy_from_user

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Tue, Feb 26, 2013 at 7:51 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think the problem is that we add the guard page *after* we do the
> normal "let's try to expand" logic.
>
> I'll take a look.

Ahh, no. The guard page logic happens later at the fault time. We do
this in two phases - first "find_extend_vma()" does what the name
claims, and then check_stack_guard_page() is done for the last-page
case from within do_anonymous_page() when we actually touch the last
page itself.

But that's actually fine. We can simply make "find_extend_vma()" do
the obvious "refuse to extend the vma all the way", because we will
later allow the guard page to extend downwards to "touch" the mapping,
but that uses separate logic. So the attached trivial patch seems to
make perfect sense:

It is totally untested, though.  Does it work for you (and we should
do the same thing for the grows-up case, obviously)?

         Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 450 bytes --]

 mm/mmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/mmap.c b/mm/mmap.c
index 318e121affda..ae47c1609d99 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2226,6 +2226,8 @@ find_extend_vma(struct mm_struct * mm, unsigned long addr)
 		return vma;
 	if (!(vma->vm_flags & VM_GROWSDOWN))
 		return NULL;
+	if (vma->vm_prev && vma->vm_prev->vm_end == addr)
+		return NULL;
 	start = vma->vm_start;
 	if (expand_stack(vma, addr))
 		return NULL;

Re: bug in generic strncpy_from_user

[Linus Torvalds]

On Tue, Feb 26, 2013 at 8:08 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It is totally untested, though.  Does it work for you (and we should
> do the same thing for the grows-up case, obviously)?

Ok, thinking about this more, this is not good enough.

There is one case where growing into the previous/next vma is
perfectly ok: when the previous/next vma is the same stack vma, and
the reason we expanded it was because we had somehow unmapped the
stack in the middle.

Which isn't common or normal, but it has been known to happen. People
doing things like unmapping sensitive data in the middle of the stack
or whatever. We explicitly allow that case for the stack guard page
for that reason.

So I'll need to extend on that logic a bit more.

                    Linus

Re: bug in generic strncpy_from_user

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Tue, Feb 26, 2013 at 3:43 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I'll need to extend on that logic a bit more.

Ok, here's the fleshed-out patch. Also fixed to use "expand_stack()",
which is what the page fault handling actually uses. And it has the
GROWSUP case as well as a comment about this all.

So it all looks good. But it's still totally and entirely untested.

Because that's how I roll. Gangsta life!

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 1520 bytes --]

 mm/mmap.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/mm/mmap.c b/mm/mmap.c
index 318e121affda..2fd9f03eee6f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2185,9 +2185,28 @@ int expand_downwards(struct vm_area_struct *vma,
 	return error;
 }
 
+/*
+ * Note how expand_stack() refuses to expand the stack all the way to
+ * abut the next virtual mapping, *unless* that mapping itself is also
+ * a stack mapping. We want to leave room for a guard page, after all
+ * (the guard page itself is not added here, that is done by the
+ * actual page faulting logic)
+ *
+ * This matches the behavior of the guard page logic (see mm/memory.c:
+ * check_stack_guard_page()), which only allows the guard page to be
+ * removed under these circumstances.
+ */
 #ifdef CONFIG_STACK_GROWSUP
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
+	struct vm_area_struct *next;
+
+	address &= PAGE_MASK;
+	next = vma->vm_next;
+	if (next && next->vm_start == address + PAGE_SIZE) {
+		if (!(next->vm_flags & VM_GROWSUP))
+			return -ENOMEM;
+	}
 	return expand_upwards(vma, address);
 }
 
@@ -2209,6 +2228,14 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
 #else
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
+	struct vm_area_struct *prev;
+
+	address &= PAGE_MASK;
+	prev = vma->vm_prev;
+	if (prev && prev->vm_end == address) {
+		if (!(prev->vm_flags & VM_GROWSDOWN))
+			return -ENOMEM;
+	}
 	return expand_downwards(vma, address);
 }
 

Re: bug in generic strncpy_from_user

[Heiko Carstens]

On Tue, Feb 26, 2013 at 04:30:50PM -0800, Linus Torvalds wrote:
> On Tue, Feb 26, 2013 at 3:43 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So I'll need to extend on that logic a bit more.
> 
> Ok, here's the fleshed-out patch. Also fixed to use "expand_stack()",
> which is what the page fault handling actually uses. And it has the
> GROWSUP case as well as a comment about this all.
> 
> So it all looks good. But it's still totally and entirely untested.

Tested with 64 bit kernel with 64 bit and 31 bit mode user space as well as
with 31 bit kernel and 31 bit user space. Each with legacy and flexible
mmap layout.
The bug is fixed and everything else still seems to work. Thanks!

Tested-By: Heiko Carstens <heiko.carstens@de.ibm.com>

Re: bug in generic strncpy_from_user

[Linus Torvalds]

On Wed, Feb 27, 2013 at 3:15 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
>
> Tested with 64 bit kernel with 64 bit and 31 bit mode user space as well as
> with 31 bit kernel and 31 bit user space. Each with legacy and flexible
> mmap layout.
> The bug is fixed and everything else still seems to work. Thanks!
>
> Tested-By: Heiko Carstens <heiko.carstens@de.ibm.com>

Ok, committed.

I'm not marking it for stable, because it doesn't seem to matter all
that much, but if there was some application that actually broke and
made you notice it that way (as opposed to you just noticing the odd
memory map), you might want to inform the stable team about commit
09884964335e.

                    Linus

Re: bug in generic strncpy_from_user

[Sedat Dilek]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, Feb 27, 2013 at 6:20 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Feb 27, 2013 at 9:15 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>
>> Where is it?
>
> Oh sorry, I hadn't pushed it out, I was looking through other emails
> in the meantime.
>
> Pushed out now (but mirror delays to public sites etc..)
>
> The patch itself is the same one I already attached earlier though
> (the last version).
>
>               Linus

I have tested the latest one against next-20130227...

- Sedat -

[-- Attachment #2: 3.8.0-next20130227-4-iniza-small.patch --]
[-- Type: application/octet-stream, Size: 8638 bytes --]

Linus Torvalds (1):
      mm: mmap: Fix bug in generic strncpy_from_user

Sedat Dilek (8):
      kbuild: deb-pkg: Try to determine distribution
      kbuild: deb-pkg: Bump year in debian/copyright file
      kbuild: deb-pkg: Update git repository URL in debian/copyright file
      Merge tag 'next-20130227' of git://git.kernel.org/.../next/linux-next into Linux-Next-v20130227
      Revert "idr: implement lookup hint"
      Merge branch 'deb-pkg-fixes' into 3.8.0-next20130227-4-iniza-small
      Merge branch 'idr-next-fixes' into 3.8.0-next20130227-4-iniza-small
      Merge branch 'mm-mmap-fixes' into 3.8.0-next20130227-4-iniza-small

 include/linux/idr.h      | 25 +------------------------
 lib/idr.c                | 38 ++++++++++++++++++++++----------------
 mm/mmap.c                | 27 +++++++++++++++++++++++++++
 scripts/package/builddeb | 19 ++++++++++++++++---
 4 files changed, 66 insertions(+), 43 deletions(-)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index aed2a0c..7b1c5c6 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -37,7 +37,6 @@ struct idr_layer {
 };
 
 struct idr {
-	struct idr_layer __rcu	*hint;	/* the last layer allocated from */
 	struct idr_layer __rcu	*top;
 	struct idr_layer	*id_free;
 	int			layers;	/* only valid w/o concurrent changes */
@@ -72,7 +71,7 @@ struct idr {
  * This is what we export.
  */
 
-void *idr_find_slowpath(struct idr *idp, int id);
+void *idr_find(struct idr *idp, int id);
 int idr_pre_get(struct idr *idp, gfp_t gfp_mask);
 int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
 void idr_preload(gfp_t gfp_mask);
@@ -98,28 +97,6 @@ static inline void idr_preload_end(void)
 }
 
 /**
- * idr_find - return pointer for given id
- * @idp: idr handle
- * @id: lookup key
- *
- * Return the pointer given the id it has been registered with.  A %NULL
- * return indicates that @id is not valid or you passed %NULL in
- * idr_get_new().
- *
- * This function can be called under rcu_read_lock(), given that the leaf
- * pointers lifetimes are correctly managed.
- */
-static inline void *idr_find(struct idr *idr, int id)
-{
-	struct idr_layer *hint = rcu_dereference_raw(idr->hint);
-
-	if ((id & ~IDR_MASK) == hint->prefix)
-		return rcu_dereference_raw(hint->ary[id & IDR_MASK]);
-
-	return idr_find_slowpath(idr, id);
-}
-
-/**
  * idr_get_new - allocate new idr entry
  * @idp: idr handle
  * @ptr: pointer you want associated with the id
diff --git a/lib/idr.c b/lib/idr.c
index 5c772dc..5cd6029 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -137,10 +137,8 @@ static void idr_layer_rcu_free(struct rcu_head *head)
 	kmem_cache_free(idr_layer_cache, layer);
 }
 
-static inline void free_layer(struct idr *idr, struct idr_layer *p)
+static inline void free_layer(struct idr_layer *p)
 {
-	if (idr->hint == p)
-		RCU_INIT_POINTER(idr->hint, NULL);
 	call_rcu(&p->rcu_head, idr_layer_rcu_free);
 }
 
@@ -365,12 +363,8 @@ build_up:
  * @id and @pa are from a successful allocation from idr_get_empty_slot().
  * Install the user pointer @ptr and mark the slot full.
  */
-static void idr_fill_slot(struct idr *idr, void *ptr, int id,
-			  struct idr_layer **pa)
+static void idr_fill_slot(void *ptr, int id, struct idr_layer **pa)
 {
-	/* update hint used for lookup, cleared from free_layer() */
-	rcu_assign_pointer(idr->hint, pa[0]);
-
 	rcu_assign_pointer(pa[0]->ary[id & IDR_MASK], (struct idr_layer *)ptr);
 	pa[0]->count++;
 	idr_mark_full(pa, id);
@@ -403,7 +397,7 @@ int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id)
 	if (rv < 0)
 		return rv == -ENOMEM ? -EAGAIN : rv;
 
-	idr_fill_slot(idp, ptr, rv, pa);
+	idr_fill_slot(ptr, rv, pa);
 	*id = rv;
 	return 0;
 }
@@ -510,7 +504,7 @@ int idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask)
 	if (unlikely(id > max))
 		return -ENOSPC;
 
-	idr_fill_slot(idr, ptr, id, pa);
+	idr_fill_slot(ptr, id, pa);
 	return id;
 }
 EXPORT_SYMBOL_GPL(idr_alloc);
@@ -547,14 +541,14 @@ static void sub_remove(struct idr *idp, int shift, int id)
 		to_free = NULL;
 		while(*paa && ! --((**paa)->count)){
 			if (to_free)
-				free_layer(idp, to_free);
+				free_layer(to_free);
 			to_free = **paa;
 			**paa-- = NULL;
 		}
 		if (!*paa)
 			idp->layers = 0;
 		if (to_free)
-			free_layer(idp, to_free);
+			free_layer(to_free);
 	} else
 		idr_remove_warning(id);
 }
@@ -587,7 +581,7 @@ void idr_remove(struct idr *idp, int id)
 		--idp->layers;
 		to_free->count = 0;
 		bitmap_clear(to_free->bitmap, 0, IDR_SIZE);
-		free_layer(idp, to_free);
+		free_layer(to_free);
 	}
 	while (idp->id_free_cnt >= MAX_IDR_FREE) {
 		p = get_from_free_list(idp);
@@ -628,7 +622,7 @@ void __idr_remove_all(struct idr *idp)
 		/* Get the highest bit that the above add changed from 0->1. */
 		while (n < fls(id ^ bt_mask)) {
 			if (p)
-				free_layer(idp, p);
+				free_layer(p);
 			n += IDR_BITS;
 			p = *--paa;
 		}
@@ -661,7 +655,19 @@ void idr_destroy(struct idr *idp)
 }
 EXPORT_SYMBOL(idr_destroy);
 
-void *idr_find_slowpath(struct idr *idp, int id)
+/**
+ * idr_find - return pointer for given id
+ * @idp: idr handle
+ * @id: lookup key
+ *
+ * Return the pointer given the id it has been registered with.  A %NULL
+ * return indicates that @id is not valid or you passed %NULL in
+ * idr_get_new().
+ *
+ * This function can be called under rcu_read_lock(), given that the leaf
+ * pointers lifetimes are correctly managed.
+ */
+void *idr_find(struct idr *idp, int id)
 {
 	int n;
 	struct idr_layer *p;
@@ -685,7 +691,7 @@ void *idr_find_slowpath(struct idr *idp, int id)
 	}
 	return((void *)p);
 }
-EXPORT_SYMBOL(idr_find_slowpath);
+EXPORT_SYMBOL(idr_find);
 
 /**
  * idr_for_each - iterate through all stored pointers
diff --git a/mm/mmap.c b/mm/mmap.c
index a5d7598..49dc7d5 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2163,9 +2163,28 @@ int expand_downwards(struct vm_area_struct *vma,
 	return error;
 }
 
+/*
+ * Note how expand_stack() refuses to expand the stack all the way to
+ * abut the next virtual mapping, *unless* that mapping itself is also
+ * a stack mapping. We want to leave room for a guard page, after all
+ * (the guard page itself is not added here, that is done by the
+ * actual page faulting logic)
+ *
+ * This matches the behavior of the guard page logic (see mm/memory.c:
+ * check_stack_guard_page()), which only allows the guard page to be
+ * removed under these circumstances.
+ */
 #ifdef CONFIG_STACK_GROWSUP
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
+	struct vm_area_struct *next;
+
+	address &= PAGE_MASK;
+	next = vma->vm_next;
+	if (next && next->vm_start == address + PAGE_SIZE) {
+		if (!(next->vm_flags & VM_GROWSUP))
+			return -ENOMEM;
+	}
 	return expand_upwards(vma, address);
 }
 
@@ -2187,6 +2206,14 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
 #else
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
+	struct vm_area_struct *prev;
+
+	address &= PAGE_MASK;
+	prev = vma->vm_prev;
+	if (prev && prev->vm_end == address) {
+		if (!(prev->vm_flags & VM_GROWSDOWN))
+			return -ENOMEM;
+	}
 	return expand_downwards(vma, address);
 }
 
diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index acb8650..7d7c9d8 100644
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -172,9 +172,22 @@ else
 fi
 maintainer="$name <$email>"
 
+# Try to determine distribution
+if [ -e $(which lsb_release) ]; then
+       codename=$(lsb_release --codename --short)
+       if [ "$codename" != "" ]; then
+		distribution=$codename
+       else
+		distribution="UNRELEASED"
+		echo "WARNING: The distribution could NOT be determined!"
+       fi
+else
+       echo "HINT: Install lsb_release binary, this helps to identify your distribution!"
+fi
+
 # Generate a simple changelog template
 cat <<EOF > debian/changelog
-linux-upstream ($packageversion) unstable; urgency=low
+linux-upstream ($packageversion) $distribution; urgency=low
 
   * Custom built Linux kernel.
 
@@ -188,10 +201,10 @@ This is a packacked upstream version of the Linux kernel.
 The sources may be found at most Linux ftp sites, including:
 ftp://ftp.kernel.org/pub/linux/kernel
 
-Copyright: 1991 - 2009 Linus Torvalds and others.
+Copyright: 1991 - 2013 Linus Torvalds and others.
 
 The git repository for mainline kernel development is at:
-git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
+git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by

Re: bug in generic strncpy_from_user

[Linus Torvalds]

On Wed, Feb 27, 2013 at 9:15 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> Where is it?

Oh sorry, I hadn't pushed it out, I was looking through other emails
in the meantime.

Pushed out now (but mirror delays to public sites etc..)

The patch itself is the same one I already attached earlier though
(the last version).

              Linus

Re: bug in generic strncpy_from_user

[Sedat Dilek]

[ QUOTE ]
On Wed, Feb 27, 2013 at 3:15 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
>
> Tested with 64 bit kernel with 64 bit and 31 bit mode user space as well as
> with 31 bit kernel and 31 bit user space. Each with legacy and flexible
> mmap layout.
> The bug is fixed and everything else still seems to work. Thanks!
>
> Tested-By: Heiko Carstens <heiko.carstens@de.ibm.com>

Ok, committed.

I'm not marking it for stable, because it doesn't seem to matter all
that much, but if there was some application that actually broke and
made you notice it that way (as opposed to you just noticing the odd
memory map), you might want to inform the stable team about commit
09884964335e.

                    Linus
[ /QUOTE ]

Where is it?

- Sedat -

http://marc.info/?l=linux-kernel&m=136198317320605&w=2