[BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Gerald Schaefer]

Hi,

elf_core_dump() does a set_fs(KERNEL_DS) and then calls vma_dump_size(),
which uses get_user() to check for an ELF header at vma->vm_start in the
user mapping. This is a bug because vm_start is a user virtual address and
get_user() will fail or even read from a kernel address (KERNEL_DS).

Maybe a get_user_pages() should be used to get the user data, or a temporary
set_fs(USER_DS)?

--
Gerald

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Andrew Morton]

On Fri, 06 Feb 2009 18:10:35 +0100
Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:

> Hi,
> 
> elf_core_dump() does a set_fs(KERNEL_DS) and then calls vma_dump_size(),
> which uses get_user() to check for an ELF header at vma->vm_start in the
> user mapping. This is a bug because vm_start is a user virtual address and
> get_user() will fail or even read from a kernel address (KERNEL_DS).
> 
> Maybe a get_user_pages() should be used to get the user data, or a temporary
> set_fs(USER_DS)?
> 

Could use __get_user() to skip the access_ok() check?

We'd need to be sure that the address isn't a kernel address or iomem
or something. 

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Linus Torvalds]

On Fri, 6 Feb 2009, Andrew Morton wrote:

> On Fri, 06 Feb 2009 18:10:35 +0100
> Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:
> > 
> > elf_core_dump() does a set_fs(KERNEL_DS) and then calls vma_dump_size(),
> > which uses get_user() to check for an ELF header at vma->vm_start in the
> > user mapping. This is a bug because vm_start is a user virtual address and
> > get_user() will fail or even read from a kernel address (KERNEL_DS).
> > 
> > Maybe a get_user_pages() should be used to get the user data, or a temporary
> > set_fs(USER_DS)?
> > 
> 
> Could use __get_user() to skip the access_ok() check?

That's not the problem.

The problem is that
 (a) some architectures actually use separate address spaces (sparc, at 
     least), so using "get_user()" while you are in KERNEL_DS will 
     literally access the wrong thing.

     __get_user doesn't affect this part.

 (b) the security one: KERNEL_DS will change the access checks that 
     get_user() does, and allow access to kernel memory.

     Using __get_user _does_ affect this part by removing the checks, but 
     since the problem was that KERNEL_DS already _weakened_ the checks, 
     removign the too-weak checking doesn't actually help.

> We'd need to be sure that the address isn't a kernel address or iomem
> or something. 

As mentioned, even this won't actually help. On at least some sparc 
machines (sparc64?), memory accesses really are segmented, with different 
address spaces for user and kernel mode, so a user and kernel address may 
actually have the exact same pointer value, but point to different memory!

So you can't just check the address. You really have to do the whole 
"set_fs()" thing to set the segment register.

		Linus

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Roland McGrath]

The address in question comes from a user vma's vm_start, so by definition
it has to be in the user part of the address space.  This code path is
excluded for VM_IO and the like by checks earlier in vma_dump_size.
So, is this problem purely theoretical?  I guess not on machines where
set_fs actually changes the meaning of the lower address space.

I think get_user_pages would certainly be overkill for this.  It's a check
to decide whether you need to pay the cost of get_user_pages, after all.

set_fs is quite cheap at least on most machines.  So a pair of set_fs calls
around that get_user call doesn't seem so bad.  OTOH, on the machines where
this actually matters at all (maybe just sparc, arm, s390?) it is
presumably (much?) more costly.  But it seems like the best solution, and
certainly is straightforward.


Thanks,
Roland

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[David Miller]

From: Roland McGrath <roland@redhat.com>
Date: Fri,  6 Feb 2009 14:07:17 -0800 (PST)

> set_fs is quite cheap at least on most machines.  So a pair of set_fs calls
> around that get_user call doesn't seem so bad.  OTOH, on the machines where
> this actually matters at all (maybe just sparc, arm, s390?) it is
> presumably (much?) more costly.  But it seems like the best solution, and
> certainly is straightforward.

On sparc set_fs() is just a privileged register write, so pretty cheap
and definitely less expensive than get_user_pages() :-)

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Linus Torvalds]

On Fri, 6 Feb 2009, Roland McGrath wrote:
> 
> set_fs is quite cheap at least on most machines.  So a pair of set_fs calls
> around that get_user call doesn't seem so bad.  OTOH, on the machines where
> this actually matters at all (maybe just sparc, arm, s390?) it is
> presumably (much?) more costly.  But it seems like the best solution, and
> certainly is straightforward.

Yes, I suspect just surrounding the load with set_fs(USER_DS) and then 
set_fs(KERNEL_DS) to put it back is likely the right thing to do.

The address is "safe" in that it does come from the vma, but we obviously 
do play games with things like the call gate mappings etc. Should we also 
perhaps do this only if the vma is marked readable and executable? That 
way we could avoid taking unnecessary faults there.

Probably doesn't really matter. 

			Linus

[PATCH] elf core dump: fix get_user use

[Roland McGrath]

The following changes since commit 6cec50838ed04a9833fb5549f698d3756bbe7e72:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../tiwai/sound-2.6

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git to-linus

Roland McGrath (1):
      elf core dump: fix get_user use

 fs/binfmt_elf.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

Thanks,
Roland

---
[PATCH] elf core dump: fix get_user use

The elf_core_dump() code does its work with set_fs(KERNEL_DS) in force,
so vma_dump_size() needs to switch back with set_fs(USER_DS) to safely
use get_user() for a normal user-space address.

Checking for VM_READ optimizes out the case where get_user() would fail
anyway.  The vm_file check here was already superfluous given the control
flow earlier in the function, so that is a cleanup/optimization unrelated
to other changes but an obvious and trivial one.

Reported-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Signed-off-by: Roland McGrath <roland@redhat.com>
---
 fs/binfmt_elf.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index e3ff2b9..33b7235 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1208,9 +1208,11 @@ static unsigned long vma_dump_size(struct vm_area_struct *vma,
 	 * check for an ELF header.  If we find one, dump the first page to
 	 * aid in determining what was mapped here.
 	 */
-	if (FILTER(ELF_HEADERS) && vma->vm_file != NULL && vma->vm_pgoff == 0) {
+	if (FILTER(ELF_HEADERS) &&
+	    vma->vm_pgoff == 0 && (vma->vm_flags & VM_READ)) {
 		u32 __user *header = (u32 __user *) vma->vm_start;
 		u32 word;
+		mm_segment_t fs = get_fs();
 		/*
 		 * Doing it this way gets the constant folded by GCC.
 		 */
@@ -1223,7 +1225,15 @@ static unsigned long vma_dump_size(struct vm_area_struct *vma,
 		magic.elfmag[EI_MAG1] = ELFMAG1;
 		magic.elfmag[EI_MAG2] = ELFMAG2;
 		magic.elfmag[EI_MAG3] = ELFMAG3;
-		if (get_user(word, header) == 0 && word == magic.cmp)
+		/*
+		 * Switch to the user "segment" for get_user(),
+		 * then put back what elf_core_dump() had in place.
+		 */
+		set_fs(USER_DS);
+		if (unlikely(get_user(word, header)))
+			word = 0;
+		set_fs(fs);
+		if (word == magic.cmp)
 			return PAGE_SIZE;
 	}
 

Re: [BUG] binfmt_elf: get_user() called in vma_dump_size() after set_fs(KERNEL_DS)

[Roland McGrath]

> Yes, I suspect just surrounding the load with set_fs(USER_DS) and then 
> set_fs(KERNEL_DS) to put it back is likely the right thing to do.

Agreed.

> The address is "safe" in that it does come from the vma, but we obviously 
> do play games with things like the call gate mappings etc. 

gate_vma has VM_ALWAYSDUMP so it never sees this whole path anyway.
I doubt there is any actual case.  But, point taken.

> Should we also perhaps do this only if the vma is marked readable and
> executable? That way we could avoid taking unnecessary faults there.
> 
> Probably doesn't really matter. 

I'm sure it doesn't really matter.  But, just to say it all: Requiring
VM_EXEC would actually exclude some valid cases.  Even requiring VM_READ is
less than perfect as far as pedantic semantics go--but there is no reason
not to check it since its lack rules out get_user() actually working
anyway.  I already chose that trade-off since get_user() here is so much
cheaper than get_user_pages().  The core dump from a stray
"mprotect(0,1UL<<32,PROT_NONE)" (i.e. presumably actually one with some
arithmetic error) would be useful if we made the check work without
VM_READ, but oh well.


Thanks,
Roland