Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[Michal Hocko]

[CCing metag people for the metag elf_map implementation specific. The thread
starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]

On Mon 16-10-17 09:39:14, Kees Cook wrote:
> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
[...]
> > +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> > +               unsigned long size, int prot, int type, unsigned long off)
> > +{
> > +       unsigned long map_addr;
> > +
> > +       /*
> > +        * If caller requests the mapping at a specific place, make sure we fail
> > +        * rather than potentially clobber an existing mapping which can have
> > +        * security consequences (e.g. smash over the stack area).
> > +        */
> > +       map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> > +       if (BAD_ADDR(map_addr))
> > +               return map_addr;
> > +
> > +       if ((type & MAP_FIXED) && map_addr != addr) {
> > +               pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> > +                               (void*)addr);
> 
> Is "info" loud enough? I actually think this should be a WARN_ONCE().

Does it make any sense to taint the kernel just because of this
condition? I can make it pr_error...

> > +               return -EAGAIN;
> > +       }
> > +
> > +       return map_addr;
> > +}
> > +
> >  static unsigned long elf_map(struct file *filep, unsigned long addr,
> >                 struct elf_phdr *eppnt, int prot, int type,
> >                 unsigned long total_size)
> > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> 
> elf_map is redirected on metag -- it should probably have its vm_mmap
> calls adjust too.

Thanks for spotting this. I am not really familiar with metag. It seems
to clear MAP_FIXED already
	tcm_tag = tcm_lookup_tag(addr);

	if (tcm_tag != TCM_INVALID_TAG)
		type &= ~MAP_FIXED;

So if there is a tag the flag is cleared. I do not understand this code
(and git log doesn't help) but why is this MAP_FIXED code really needed?

> >         */
> >         if (total_size) {
> >                 total_size = ELF_PAGEALIGN(total_size);
> > -               map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> > +               map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
> >                 if (!BAD_ADDR(map_addr))
> >                         vm_munmap(map_addr+size, total_size-size);
> >         } else
> > -               map_addr = vm_mmap(filep, addr, size, prot, type, off);
> > +               map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
> >
> >         return(map_addr);
> >  }
> > @@ -1215,7 +1238,7 @@ static int load_elf_library(struct file *file)
> >                 eppnt++;
> >
> >         /* Now use mmap to map the library into memory. */
> > -       error = vm_mmap(file,
> > +       error = elf_vm_mmap(file,
> >                         ELF_PAGESTART(eppnt->p_vaddr),
> >                         (eppnt->p_filesz +
> >                          ELF_PAGEOFFSET(eppnt->p_vaddr)),
> > --
> > 2.14.2
> >
> 
> Otherwise, yeah, this should be good.

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[James Hogan]

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

On Mon, Oct 16, 2017 at 09:00:47PM +0200, Michal Hocko wrote:
> [CCing metag people for the metag elf_map implementation specific. The thread
> starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko@kernel.org]
> 
> On Mon 16-10-17 09:39:14, Kees Cook wrote:
> > On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > +               return -EAGAIN;
> > > +       }
> > > +
> > > +       return map_addr;
> > > +}
> > > +
> > >  static unsigned long elf_map(struct file *filep, unsigned long addr,
> > >                 struct elf_phdr *eppnt, int prot, int type,
> > >                 unsigned long total_size)
> > > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> > 
> > elf_map is redirected on metag -- it should probably have its vm_mmap
> > calls adjust too.
> 
> Thanks for spotting this. I am not really familiar with metag. It seems
> to clear MAP_FIXED already
> 	tcm_tag = tcm_lookup_tag(addr);
> 
> 	if (tcm_tag != TCM_INVALID_TAG)
> 		type &= ~MAP_FIXED;
> 
> So if there is a tag the flag is cleared. I do not understand this code
> (and git log doesn't help) but why is this MAP_FIXED code really needed?

This function was added to the metag port in mid-2010 to support ELFs
with tightly coupled memory (TCM) segments, for example metag "core"
memories are at fixed virtual addresses and aren't MMU mappable (i.e.
globally accessible), and are outside of the usual userland address
range, but are as fast as cache. The commit message says this:

> Override the definition of the elf_map() function to special case
> sections that are loaded at the address of the internal memories.
> If we have such a section, map it at a different address and copy
> the contents of the section into the appropriate memory.

So yeh, it looks like if the section is meant to use TCM based on the
virtual address, it drops MAP_FIXED so that the vm_mmap can succeed
(because its outside the normally valid range), and then copies it
directly to the desired TCM so the program can use it.

Hope that helps add some context to understand whats needed.

There was some description of this in an ELCE-2010 talk by the original
author Will Newton that may also be of interest [1].

Cheers
James

[1] http://free-electrons.com/blog/elce-2010-videos/
    "Exploiting On-chip Memories in Embedded Linux Applications"
    See slides about "core memories".

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[Michal Hocko]

On Mon 16-10-17 21:02:09, James Hogan wrote:
> On Mon, Oct 16, 2017 at 09:00:47PM +0200, Michal Hocko wrote:
> > [CCing metag people for the metag elf_map implementation specific. The thread
> > starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > 
> > On Mon 16-10-17 09:39:14, Kees Cook wrote:
> > > On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > > > +               return -EAGAIN;
> > > > +       }
> > > > +
> > > > +       return map_addr;
> > > > +}
> > > > +
> > > >  static unsigned long elf_map(struct file *filep, unsigned long addr,
> > > >                 struct elf_phdr *eppnt, int prot, int type,
> > > >                 unsigned long total_size)
> > > > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> > > 
> > > elf_map is redirected on metag -- it should probably have its vm_mmap
> > > calls adjust too.
> > 
> > Thanks for spotting this. I am not really familiar with metag. It seems
> > to clear MAP_FIXED already
> > 	tcm_tag = tcm_lookup_tag(addr);
> > 
> > 	if (tcm_tag != TCM_INVALID_TAG)
> > 		type &= ~MAP_FIXED;
> > 
> > So if there is a tag the flag is cleared. I do not understand this code
> > (and git log doesn't help) but why is this MAP_FIXED code really needed?
> 
> This function was added to the metag port in mid-2010 to support ELFs
> with tightly coupled memory (TCM) segments, for example metag "core"
> memories are at fixed virtual addresses and aren't MMU mappable (i.e.
> globally accessible), and are outside of the usual userland address
> range, but are as fast as cache. The commit message says this:
> 
> > Override the definition of the elf_map() function to special case
> > sections that are loaded at the address of the internal memories.
> > If we have such a section, map it at a different address and copy
> > the contents of the section into the appropriate memory.
> 
> So yeh, it looks like if the section is meant to use TCM based on the
> virtual address, it drops MAP_FIXED so that the vm_mmap can succeed
> (because its outside the normally valid range), and then copies it
> directly to the desired TCM so the program can use it.
> 
> Hope that helps add some context to understand whats needed.

Hmm, so IIUC then we need the same fix as
http://lkml.kernel.org/r/20171016134446.19910-2-mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
right?

This would be something like. I wanted to share elf_vm_mmap but didn't
find a proper place to not cause include dependency hell so I balied out
to c&p.
---
diff --git a/arch/metag/kernel/process.c b/arch/metag/kernel/process.c
index c4606ce743d2..b20596b4c4c2 100644
--- a/arch/metag/kernel/process.c
+++ b/arch/metag/kernel/process.c
@@ -378,6 +378,29 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu)
 
 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
 
+static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
+		unsigned long size, int prot, int type, unsigned long off)
+{
+	unsigned long map_addr;
+
+	/*
+	 * If caller requests the mapping at a specific place, make sure we fail
+	 * rather than potentially clobber an existing mapping which can have
+	 * security consequences (e.g. smash over the stack area).
+	 */
+	map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
+	if (BAD_ADDR(map_addr))
+		return map_addr;
+
+	if ((type & MAP_FIXED) && map_addr != addr) {
+		pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
+				(void*)addr);
+		return -EAGAIN;
+	}
+
+	return map_addr;
+}
+
 unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
 			      struct elf_phdr *eppnt, int prot, int type,
 			      unsigned long total_size)
@@ -410,11 +433,11 @@ unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
 	*/
 	if (total_size) {
 		total_size = ELF_PAGEALIGN(total_size);
-		map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
+		map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
 		if (!BAD_ADDR(map_addr))
 			vm_munmap(map_addr+size, total_size-size);
 	} else
-		map_addr = vm_mmap(filep, addr, size, prot, type, off);
+		map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
 
 	if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) {
 		struct tcm_allocation *tcm;

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[James Hogan]

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

On Tue, Oct 17, 2017 at 09:37:48AM +0200, Michal Hocko wrote:
> On Mon 16-10-17 21:02:09, James Hogan wrote:
> > On Mon, Oct 16, 2017 at 09:00:47PM +0200, Michal Hocko wrote:
> > > [CCing metag people for the metag elf_map implementation specific. The thread
> > > starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko@kernel.org]
> > > 
> > > On Mon 16-10-17 09:39:14, Kees Cook wrote:
> > > > On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > > > +               return -EAGAIN;
> > > > > +       }
> > > > > +
> > > > > +       return map_addr;
> > > > > +}
> > > > > +
> > > > >  static unsigned long elf_map(struct file *filep, unsigned long addr,
> > > > >                 struct elf_phdr *eppnt, int prot, int type,
> > > > >                 unsigned long total_size)
> > > > > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> > > > 
> > > > elf_map is redirected on metag -- it should probably have its vm_mmap
> > > > calls adjust too.
> > > 
> > > Thanks for spotting this. I am not really familiar with metag. It seems
> > > to clear MAP_FIXED already
> > > 	tcm_tag = tcm_lookup_tag(addr);
> > > 
> > > 	if (tcm_tag != TCM_INVALID_TAG)
> > > 		type &= ~MAP_FIXED;
> > > 
> > > So if there is a tag the flag is cleared. I do not understand this code
> > > (and git log doesn't help) but why is this MAP_FIXED code really needed?
> > 
> > This function was added to the metag port in mid-2010 to support ELFs
> > with tightly coupled memory (TCM) segments, for example metag "core"
> > memories are at fixed virtual addresses and aren't MMU mappable (i.e.
> > globally accessible), and are outside of the usual userland address
> > range, but are as fast as cache. The commit message says this:
> > 
> > > Override the definition of the elf_map() function to special case
> > > sections that are loaded at the address of the internal memories.
> > > If we have such a section, map it at a different address and copy
> > > the contents of the section into the appropriate memory.
> > 
> > So yeh, it looks like if the section is meant to use TCM based on the
> > virtual address, it drops MAP_FIXED so that the vm_mmap can succeed
> > (because its outside the normally valid range), and then copies it
> > directly to the desired TCM so the program can use it.
> > 
> > Hope that helps add some context to understand whats needed.
> 
> Hmm, so IIUC then we need the same fix as
> http://lkml.kernel.org/r/20171016134446.19910-2-mhocko@kernel.org,
> right?
> 
> This would be something like. I wanted to share elf_vm_mmap but didn't
> find a proper place to not cause include dependency hell so I balied out
> to c&p.
> ---
> diff --git a/arch/metag/kernel/process.c b/arch/metag/kernel/process.c
> index c4606ce743d2..b20596b4c4c2 100644
> --- a/arch/metag/kernel/process.c
> +++ b/arch/metag/kernel/process.c
> @@ -378,6 +378,29 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu)
>  
>  #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
>  
> +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> +		unsigned long size, int prot, int type, unsigned long off)
> +{
> +	unsigned long map_addr;
> +
> +	/*
> +	 * If caller requests the mapping at a specific place, make sure we fail
> +	 * rather than potentially clobber an existing mapping which can have
> +	 * security consequences (e.g. smash over the stack area).
> +	 */
> +	map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> +	if (BAD_ADDR(map_addr))
> +		return map_addr;
> +
> +	if ((type & MAP_FIXED) && map_addr != addr) {
> +		pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> +				(void*)addr);
> +		return -EAGAIN;
> +	}
> +
> +	return map_addr;
> +}
> +
>  unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
>  			      struct elf_phdr *eppnt, int prot, int type,
>  			      unsigned long total_size)
> @@ -410,11 +433,11 @@ unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
>  	*/
>  	if (total_size) {
>  		total_size = ELF_PAGEALIGN(total_size);
> -		map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> +		map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
>  		if (!BAD_ADDR(map_addr))
>  			vm_munmap(map_addr+size, total_size-size);
>  	} else
> -		map_addr = vm_mmap(filep, addr, size, prot, type, off);
> +		map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
>  
>  	if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) {
>  		struct tcm_allocation *tcm;

Yeh that looks reasonable to me.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[Michal Hocko]

On Tue 17-10-17 09:35:57, James Hogan wrote:
> On Tue, Oct 17, 2017 at 09:37:48AM +0200, Michal Hocko wrote:
[...]
> > This would be something like. I wanted to share elf_vm_mmap but didn't
> > find a proper place to not cause include dependency hell so I balied out
> > to c&p.
> > ---
> > diff --git a/arch/metag/kernel/process.c b/arch/metag/kernel/process.c
> > index c4606ce743d2..b20596b4c4c2 100644
> > --- a/arch/metag/kernel/process.c
> > +++ b/arch/metag/kernel/process.c
> > @@ -378,6 +378,29 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu)
> >  
> >  #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> >  
> > +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> > +		unsigned long size, int prot, int type, unsigned long off)
> > +{
> > +	unsigned long map_addr;
> > +
> > +	/*
> > +	 * If caller requests the mapping at a specific place, make sure we fail
> > +	 * rather than potentially clobber an existing mapping which can have
> > +	 * security consequences (e.g. smash over the stack area).
> > +	 */
> > +	map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> > +	if (BAD_ADDR(map_addr))
> > +		return map_addr;
> > +
> > +	if ((type & MAP_FIXED) && map_addr != addr) {
> > +		pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> > +				(void*)addr);
> > +		return -EAGAIN;
> > +	}
> > +
> > +	return map_addr;
> > +}
> > +
> >  unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
> >  			      struct elf_phdr *eppnt, int prot, int type,
> >  			      unsigned long total_size)
> > @@ -410,11 +433,11 @@ unsigned long __metag_elf_map(struct file *filep, unsigned long addr,
> >  	*/
> >  	if (total_size) {
> >  		total_size = ELF_PAGEALIGN(total_size);
> > -		map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> > +		map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
> >  		if (!BAD_ADDR(map_addr))
> >  			vm_munmap(map_addr+size, total_size-size);
> >  	} else
> > -		map_addr = vm_mmap(filep, addr, size, prot, type, off);
> > +		map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
> >  
> >  	if (!BAD_ADDR(map_addr) && tcm_tag != TCM_INVALID_TAG) {
> >  		struct tcm_allocation *tcm;
> 
> Yeh that looks reasonable to me.

Thanks for double checking. I will make sure to CC you when reposting
the patch after other concerns sort out.

-- 
Michal Hocko
SUSE Labs