[PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[graff.yang]

From: Graff Yang <graf.yang@analog.com>

The original code calling security_file_mmap() use user's hint address
as it's 5th argument(addr). This is improper, as the hint address may be
NULL.
In this case, the security_file_mmap() may incorrectly return -EPERM.

This patch moved the calling of security_file_mmap() out of
validate_mmap_request() to do_mmap_pgoff(), and call this
security API with the address that attempting to mmap.

Signed-off-by: Graff Yang <graf.yang@analog.com>
---
 mm/nommu.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index 3a5e989..fc986d4 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -862,7 +862,6 @@ static int validate_mmap_request(struct file *file,
 				 unsigned long *_capabilities)
 {
 	unsigned long capabilities, rlen;
-	unsigned long reqprot = prot;
 	int ret;
 
 	/* do the simple checks first */
@@ -1013,11 +1012,6 @@ static int validate_mmap_request(struct file *file,
 			prot |= PROT_EXEC;
 	}
 
-	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
-	if (ret < 0)
-		return ret;
-
 	/* looks okay */
 	*_capabilities = capabilities;
 	return 0;
@@ -1231,6 +1225,7 @@ unsigned long do_mmap_pgoff(struct file *file,
 	struct vm_area_struct *vma;
 	struct vm_region *region;
 	struct rb_node *rb;
+	unsigned long reqprot = prot;
 	unsigned long capabilities, vm_flags, result;
 	int ret;
 
@@ -1327,6 +1322,12 @@ unsigned long do_mmap_pgoff(struct file *file,
 				continue;
 			}
 
+			/* allow the security API to have its say */
+			ret = security_file_mmap(file, reqprot, prot, flags,
+							pregion->vm_start, 0);
+			if (ret < 0)
+				goto error_just_free;
+
 			/* we've found a region we can share */
 			atomic_inc(&pregion->vm_usage);
 			vma->vm_region = pregion;
@@ -1394,6 +1395,11 @@ unsigned long do_mmap_pgoff(struct file *file,
 	if (ret < 0)
 		goto error_put_region;
 
+	ret = security_file_mmap(file, reqprot, prot, flags,
+					vma->vm_start, 0);
+	if (ret < 0)
+		goto error_put_region;
+
 	/* okay... we have a mapping; now we have to register it */
 	result = vma->vm_start;
 
-- 
1.6.4.4

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

<graff.yang@gmail.com> wrote:

> The original code calling security_file_mmap() use user's hint address
> as it's 5th argument(addr). This is improper, as the hint address may be
> NULL.
> In this case, the security_file_mmap() may incorrectly return -EPERM.
> 
> This patch moved the calling of security_file_mmap() out of
> validate_mmap_request() to do_mmap_pgoff(), and call this
> security API with the address that attempting to mmap.

I think this is the wrong approach.  Firstly, the hint is cleared in NOMMU
mode, and secondly, I think that the check against the minimum LSM address is
pointless in NOMMU mode too.

So I think the attached patch is a better approach.

David
---
From: David Howells <dhowells@redhat.com>

NOMMU: Ignore the address parameter in the file_mmap() security check

Ignore the address parameter in the various file_mmap() security checks when
CONFIG_MMU=n as the address hint is ignored under those circumstances, and in
any case the minimum mapping address check is pointless in NOMMU mode.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/security.h |    1 +
 mm/nommu.c               |    2 +-
 security/commoncap.c     |    2 ++
 security/selinux/hooks.c |    2 ++
 4 files changed, 6 insertions(+), 1 deletions(-)


diff --git a/include/linux/security.h b/include/linux/security.h
index 239e40d..0583f16 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -593,6 +593,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@reqprot contains the protection requested by the application.
  *	@prot contains the protection that will be applied by the kernel.
  *	@flags contains the operational flags.
+ *	@addr contains the mapping address, and should be ignored in NOMMU mode.
  *	Return 0 if permission is granted.
  * @file_mprotect:
  *	Check permissions before changing memory access permissions.
diff --git a/mm/nommu.c b/mm/nommu.c
index 3c3b4b2..cfea46c 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -974,7 +974,7 @@ static int validate_mmap_request(struct file *file,
 	}
 
 	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	ret = security_file_mmap(file, reqprot, prot, flags, 0, 0);
 	if (ret < 0)
 		return ret;
 
diff --git a/security/commoncap.c b/security/commoncap.c
index fe30751..ac1f745 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1005,6 +1005,7 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
 {
 	int ret = 0;
 
+#ifdef CONFIG_MMU
 	if (addr < dac_mmap_min_addr) {
 		ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
 				  SECURITY_CAP_AUDIT);
@@ -1012,5 +1013,6 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
 		if (ret == 0)
 			current->flags |= PF_SUPERPRIV;
 	}
+#endif
 	return ret;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bb230d5..93d61f8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3046,6 +3046,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 			     unsigned long addr, unsigned long addr_only)
 {
 	int rc = 0;
+#ifdef CONFIG_MMU
 	u32 sid = current_sid();
 
 	/*
@@ -3060,6 +3061,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 		if (rc)
 			return rc;
 	}
+#endif
 
 	/* do DAC check on address space usage */
 	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[graff yang]

Hi, David,
Thanks your patch, I will test it.


On Wed, Oct 14, 2009 at 10:08 PM, David Howells <dhowells@redhat.com> wrote:
> <graff.yang@gmail.com> wrote:
>
>> The original code calling security_file_mmap() use user's hint address
>> as it's 5th argument(addr). This is improper, as the hint address may be
>> NULL.
>> In this case, the security_file_mmap() may incorrectly return -EPERM.
>>
>> This patch moved the calling of security_file_mmap() out of
>> validate_mmap_request() to do_mmap_pgoff(), and call this
>> security API with the address that attempting to mmap.
>
> I think this is the wrong approach.  Firstly, the hint is cleared in NOMMU
> mode, and secondly, I think that the check against the minimum LSM address is
> pointless in NOMMU mode too.
>
> So I think the attached patch is a better approach.
>
> David
> ---
> From: David Howells <dhowells@redhat.com>
>
> NOMMU: Ignore the address parameter in the file_mmap() security check
>
> Ignore the address parameter in the various file_mmap() security checks when
> CONFIG_MMU=n as the address hint is ignored under those circumstances, and in
> any case the minimum mapping address check is pointless in NOMMU mode.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  include/linux/security.h |    1 +
>  mm/nommu.c               |    2 +-
>  security/commoncap.c     |    2 ++
>  security/selinux/hooks.c |    2 ++
>  4 files changed, 6 insertions(+), 1 deletions(-)
>
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 239e40d..0583f16 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -593,6 +593,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>  *     @reqprot contains the protection requested by the application.
>  *     @prot contains the protection that will be applied by the kernel.
>  *     @flags contains the operational flags.
> + *     @addr contains the mapping address, and should be ignored in NOMMU mode.
>  *     Return 0 if permission is granted.
>  * @file_mprotect:
>  *     Check permissions before changing memory access permissions.
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 3c3b4b2..cfea46c 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -974,7 +974,7 @@ static int validate_mmap_request(struct file *file,
>        }
>
>        /* allow the security API to have its say */
> -       ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
> +       ret = security_file_mmap(file, reqprot, prot, flags, 0, 0);
>        if (ret < 0)
>                return ret;
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index fe30751..ac1f745 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1005,6 +1005,7 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
>  {
>        int ret = 0;
>
> +#ifdef CONFIG_MMU
>        if (addr < dac_mmap_min_addr) {
>                ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
>                                  SECURITY_CAP_AUDIT);
> @@ -1012,5 +1013,6 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
>                if (ret == 0)
>                        current->flags |= PF_SUPERPRIV;
>        }
> +#endif
>        return ret;
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index bb230d5..93d61f8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3046,6 +3046,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
>                             unsigned long addr, unsigned long addr_only)
>  {
>        int rc = 0;
> +#ifdef CONFIG_MMU
>        u32 sid = current_sid();
>
>        /*
> @@ -3060,6 +3061,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
>                if (rc)
>                        return rc;
>        }
> +#endif
>
>        /* do DAC check on address space usage */
>        rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
>



-- 
-Graff

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[graff yang]

Hi, David,

Your patch works both with SELINUX enabled or disabled.
But, how to prevent the address that attempting to be mapped to be lower
than CONFIG_LSM_MMAP_MIN_ADDR/CONFIG_DEFAULT_MMAP_MIN_ADDR?
This is what the security_file_mmap() is doing and mmu's
do_mmap_pgoff() has implemented.

On Thu, Oct 15, 2009 at 10:21 AM, graff yang <graff.yang@gmail.com> wrote:
> Hi, David,
> Thanks your patch, I will test it.
>
>
> On Wed, Oct 14, 2009 at 10:08 PM, David Howells <dhowells@redhat.com> wrote:
>> <graff.yang@gmail.com> wrote:
>>
>>> The original code calling security_file_mmap() use user's hint address
>>> as it's 5th argument(addr). This is improper, as the hint address may be
>>> NULL.
>>> In this case, the security_file_mmap() may incorrectly return -EPERM.
>>>
>>> This patch moved the calling of security_file_mmap() out of
>>> validate_mmap_request() to do_mmap_pgoff(), and call this
>>> security API with the address that attempting to mmap.
>>
>> I think this is the wrong approach.  Firstly, the hint is cleared in NOMMU
>> mode, and secondly, I think that the check against the minimum LSM address is
>> pointless in NOMMU mode too.
>>
>> So I think the attached patch is a better approach.
>>
>> David
>> ---
>> From: David Howells <dhowells@redhat.com>
>>
>> NOMMU: Ignore the address parameter in the file_mmap() security check
>>
>> Ignore the address parameter in the various file_mmap() security checks when
>> CONFIG_MMU=n as the address hint is ignored under those circumstances, and in
>> any case the minimum mapping address check is pointless in NOMMU mode.
>>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>>  include/linux/security.h |    1 +
>>  mm/nommu.c               |    2 +-
>>  security/commoncap.c     |    2 ++
>>  security/selinux/hooks.c |    2 ++
>>  4 files changed, 6 insertions(+), 1 deletions(-)
>>
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 239e40d..0583f16 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -593,6 +593,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>>  *     @reqprot contains the protection requested by the application.
>>  *     @prot contains the protection that will be applied by the kernel.
>>  *     @flags contains the operational flags.
>> + *     @addr contains the mapping address, and should be ignored in NOMMU mode.
>>  *     Return 0 if permission is granted.
>>  * @file_mprotect:
>>  *     Check permissions before changing memory access permissions.
>> diff --git a/mm/nommu.c b/mm/nommu.c
>> index 3c3b4b2..cfea46c 100644
>> --- a/mm/nommu.c
>> +++ b/mm/nommu.c
>> @@ -974,7 +974,7 @@ static int validate_mmap_request(struct file *file,
>>        }
>>
>>        /* allow the security API to have its say */
>> -       ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
>> +       ret = security_file_mmap(file, reqprot, prot, flags, 0, 0);
>>        if (ret < 0)
>>                return ret;
>>
>> diff --git a/security/commoncap.c b/security/commoncap.c
>> index fe30751..ac1f745 100644
>> --- a/security/commoncap.c
>> +++ b/security/commoncap.c
>> @@ -1005,6 +1005,7 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
>>  {
>>        int ret = 0;
>>
>> +#ifdef CONFIG_MMU
>>        if (addr < dac_mmap_min_addr) {
>>                ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
>>                                  SECURITY_CAP_AUDIT);
>> @@ -1012,5 +1013,6 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
>>                if (ret == 0)
>>                        current->flags |= PF_SUPERPRIV;
>>        }
>> +#endif
>>        return ret;
>>  }
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index bb230d5..93d61f8 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3046,6 +3046,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
>>                             unsigned long addr, unsigned long addr_only)
>>  {
>>        int rc = 0;
>> +#ifdef CONFIG_MMU
>>        u32 sid = current_sid();
>>
>>        /*
>> @@ -3060,6 +3061,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
>>                if (rc)
>>                        return rc;
>>        }
>> +#endif
>>
>>        /* do DAC check on address space usage */
>>        rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
>>
>
>
>
> --
> -Graff
>



-- 
-Graff

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

graff yang <graff.yang@gmail.com> wrote:

> Your patch works both with SELINUX enabled or disabled.

Thanks.

> But, how to prevent the address that attempting to be mapped to be lower
> than CONFIG_LSM_MMAP_MIN_ADDR/CONFIG_DEFAULT_MMAP_MIN_ADDR?
> This is what the security_file_mmap() is doing and mmu's
> do_mmap_pgoff() has implemented.

You need to ask yourself two questions:

 (1) Does the test make any sense in the NOMMU context?  Given that the
     userspace program _cannot_ specify that something should be mapped below
     that address (since MAP_FIXED gives an error and the hint is ignored),
     I'd say not.

 (2) Is it likely that LSM security would be used with NOMMU anyway, given
     that there is nothing stopping userspace programs editing the kernel
     directly?

You need to show that it makes sense to do the test in a NOMMU context.
Remember, if there is no physical medium at address 0 (RAM, flash, whatever),
you can't map anything there.  This might best be left to the arch to not
provide page 0 of RAM to the page allocator during memory initialisation.

David

Re: [Uclinux-dist-devel] [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Mike Frysinger]

On Wed, Oct 14, 2009 at 10:08, David Howells wrote:
> <graff.yang@gmail.com> wrote:
>> The original code calling security_file_mmap() use user's hint address
>> as it's 5th argument(addr). This is improper, as the hint address may be
>> NULL.
>> In this case, the security_file_mmap() may incorrectly return -EPERM.
>>
>> This patch moved the calling of security_file_mmap() out of
>> validate_mmap_request() to do_mmap_pgoff(), and call this
>> security API with the address that attempting to mmap.
>
> I think this is the wrong approach.  Firstly, the hint is cleared in NOMMU
> mode, and secondly, I think that the check against the minimum LSM address is
> pointless in NOMMU mode too.
>
> So I think the attached patch is a better approach.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Wed, Oct 14, 2009 at 10:08 AM, David Howells <dhowells@redhat.com> wrote:
> <graff.yang@gmail.com> wrote:
>
>> The original code calling security_file_mmap() use user's hint address
>> as it's 5th argument(addr). This is improper, as the hint address may be
>> NULL.
>> In this case, the security_file_mmap() may incorrectly return -EPERM.
>>
>> This patch moved the calling of security_file_mmap() out of
>> validate_mmap_request() to do_mmap_pgoff(), and call this
>> security API with the address that attempting to mmap.
>
> I think this is the wrong approach.  Firstly, the hint is cleared in NOMMU
> mode, and secondly, I think that the check against the minimum LSM address is
> pointless in NOMMU mode too.

I really don't like seeing such irrelevant (that's not the right word,
but I can't think what is) ifdefs creeping down into the security
layer as LSM authors are likely to mess them up in the future.  I'd
probably rather see the addr_only argument changed into a flags field.
 One for addr_only and one flag for not_addr.  The nommu case could
just set the not_addr flag and it's obvious how the LSMs (or
capabilities if !CONFIG_SECURITY) should handle it, also works if some
other future need arises...

-Eric

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

Eric Paris <eparis@parisplace.org> wrote:

> I really don't like seeing such irrelevant (that's not the right word,
> but I can't think what is) ifdefs creeping down into the security
> layer as LSM authors are likely to mess them up in the future.  I'd
> probably rather see the addr_only argument changed into a flags field.
>  One for addr_only and one flag for not_addr.  The nommu case could
> just set the not_addr flag and it's obvious how the LSMs (or
> capabilities if !CONFIG_SECURITY) should handle it, also works if some
> other future need arises...

A better way still, might be to deny the possibility of CONFIG_SECURITY if
CONFIG_MMU=n.  After all, security is sort of pointless when a userspace
program can just edit the kernel at a whim.

David

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Fri, 2009-10-16 at 16:14 +0100, David Howells wrote:
> Eric Paris <eparis@parisplace.org> wrote:
> 
> > I really don't like seeing such irrelevant (that's not the right word,
> > but I can't think what is) ifdefs creeping down into the security
> > layer as LSM authors are likely to mess them up in the future.  I'd
> > probably rather see the addr_only argument changed into a flags field.
> >  One for addr_only and one flag for not_addr.  The nommu case could
> > just set the not_addr flag and it's obvious how the LSMs (or
> > capabilities if !CONFIG_SECURITY) should handle it, also works if some
> > other future need arises...
> 
> A better way still, might be to deny the possibility of CONFIG_SECURITY if
> CONFIG_MMU=n.  After all, security is sort of pointless when a userspace
> program can just edit the kernel at a whim.

That would still call cap_file_mmap() and wouldn't solve your problem.

-Eric

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

Eric Paris <eparis@redhat.com> wrote:

> That would still call cap_file_mmap() and wouldn't solve your problem.

Hmmm...  I guess I don't see the problem occur because I always run the
programs as root.

I would guess that cap_file_mmap() and selinux_file_mmap() are, perhaps, too
strict.  The hint shouldn't be rejected unless MAP_FIXED is also set, surely,
but should rather be revised upwards.

Certainly, addr==NULL and !MAP_FIXED is a reasonable case to permit, even in
tightly secured MMU and SELinux mode...  After all, the manual page says:

	If addr is NULL, then the kernel chooses the address at which to create
	the mapping; this is the most portable method of creating  a  new  map-
	ping.

David

Re: [Uclinux-dist-devel] [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Mike Frysinger]

On Fri, Oct 16, 2009 at 11:14, David Howells wrote:
> Eric Paris <eparis@parisplace.org> wrote:
>> I really don't like seeing such irrelevant (that's not the right word,
>> but I can't think what is) ifdefs creeping down into the security
>> layer as LSM authors are likely to mess them up in the future.  I'd
>> probably rather see the addr_only argument changed into a flags field.
>>  One for addr_only and one flag for not_addr.  The nommu case could
>> just set the not_addr flag and it's obvious how the LSMs (or
>> capabilities if !CONFIG_SECURITY) should handle it, also works if some
>> other future need arises...
>
> A better way still, might be to deny the possibility of CONFIG_SECURITY if
> CONFIG_MMU=n.  After all, security is sort of pointless when a userspace
> program can just edit the kernel at a whim.

except for MPU users, and arent some security restrictions useful when
talking about networked daemons ?
-mike

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Fri, 2009-10-16 at 16:43 +0100, David Howells wrote:
> Eric Paris <eparis@redhat.com> wrote:
> 
> > That would still call cap_file_mmap() and wouldn't solve your problem.
> 
> Hmmm...  I guess I don't see the problem occur because I always run the
> programs as root.

most likely yes, your processes are going to have CAP_SYS_RAWIO and will
happily sail through the cap_file_mmap().

> I would guess that cap_file_mmap() and selinux_file_mmap() are, perhaps, too
> strict.  The hint shouldn't be rejected unless MAP_FIXED is also set, surely,
> but should rather be revised upwards.

On the mmu side we don't check until the kernel has turned the hint into
a real address.  Which is apparently different then where the hook was
originally placed in the nommu side.

> Certainly, addr==NULL and !MAP_FIXED is a reasonable case to permit, even in
> tightly secured MMU and SELinux mode...  After all, the manual page says:
> 
> 	If addr is NULL, then the kernel chooses the address at which to create
> 	the mapping; this is the most portable method of creating  a  new  map-
> 	ping.

I agree, I think the choices are

A) agree that we just shouldn't check the address on nommu
   A1) change kconfig to not allow the setting of these
	(easy to do for the LSM check, but not easy as it stands today
	 for the cap_file_mmap() check)
   A2) change the addr_only to be a flags which indicate not to check
	(my original suggestion)
   A3) push the config_nommu down into the security code
	(your patch)
B) actually check the address, which requires moving the hook to a
location where it has been resolved.
	(graff yang's patch)

I'll leave it up to you to determine which you like the best since you
know the implications of nommu.

-Eric

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Andrew Morton]

On Fri, 16 Oct 2009 11:55:29 -0400
Eric Paris <eparis@redhat.com> wrote:

> On Fri, 2009-10-16 at 16:43 +0100, David Howells wrote:
> > Eric Paris <eparis@redhat.com> wrote:
> > 
> > > That would still call cap_file_mmap() and wouldn't solve your problem.
> > 
> > Hmmm...  I guess I don't see the problem occur because I always run the
> > programs as root.
> 
> most likely yes, your processes are going to have CAP_SYS_RAWIO and will
> happily sail through the cap_file_mmap().
> 
> > I would guess that cap_file_mmap() and selinux_file_mmap() are, perhaps, too
> > strict.  The hint shouldn't be rejected unless MAP_FIXED is also set, surely,
> > but should rather be revised upwards.
> 
> On the mmu side we don't check until the kernel has turned the hint into
> a real address.  Which is apparently different then where the hook was
> originally placed in the nommu side.
> 
> > Certainly, addr==NULL and !MAP_FIXED is a reasonable case to permit, even in
> > tightly secured MMU and SELinux mode...  After all, the manual page says:
> > 
> > 	If addr is NULL, then the kernel chooses the address at which to create
> > 	the mapping; this is the most portable method of creating  a  new  map-
> > 	ping.
> 
> I agree, I think the choices are
> 
> A) agree that we just shouldn't check the address on nommu
>    A1) change kconfig to not allow the setting of these
> 	(easy to do for the LSM check, but not easy as it stands today
> 	 for the cap_file_mmap() check)
>    A2) change the addr_only to be a flags which indicate not to check
> 	(my original suggestion)
>    A3) push the config_nommu down into the security code
> 	(your patch)
> B) actually check the address, which requires moving the hook to a
> location where it has been resolved.
> 	(graff yang's patch)
> 
> I'll leave it up to you to determine which you like the best since you
> know the implications of nommu.
> 

So what's the status of this issue now?  Should I merge the below into
2.6.32?

Thanks.



From: David Howells <dhowells@redhat.com>

Ignore the address parameter in the various file_mmap() security checks
when CONFIG_MMU=n as the address hint is ignored under those
circumstances, and in any case the minimum mapping address check is
pointless in NOMMU mode.

Signed-off-by: David Howells <dhowells@redhat.com>
Reported-by: Graff Yang <graf.yang@analog.com>
Cc: James Morris <jmorris@namei.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/security.h |    1 +
 mm/nommu.c               |    2 +-
 security/commoncap.c     |    2 ++
 security/selinux/hooks.c |    2 ++
 4 files changed, 6 insertions(+), 1 deletion(-)

diff -puN include/linux/security.h~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check include/linux/security.h
--- a/include/linux/security.h~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check
+++ a/include/linux/security.h
@@ -609,6 +609,7 @@ static inline void security_free_mnt_opt
  *	@reqprot contains the protection requested by the application.
  *	@prot contains the protection that will be applied by the kernel.
  *	@flags contains the operational flags.
+ *	@addr contains the mapping address, and should be ignored in NOMMU mode.
  *	Return 0 if permission is granted.
  * @file_mprotect:
  *	Check permissions before changing memory access permissions.
diff -puN mm/nommu.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check mm/nommu.c
--- a/mm/nommu.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check
+++ a/mm/nommu.c
@@ -974,7 +974,7 @@ static int validate_mmap_request(struct 
 	}
 
 	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	ret = security_file_mmap(file, reqprot, prot, flags, 0, 0);
 	if (ret < 0)
 		return ret;
 
diff -puN security/commoncap.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check security/commoncap.c
--- a/security/commoncap.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check
+++ a/security/commoncap.c
@@ -1005,6 +1005,7 @@ int cap_file_mmap(struct file *file, uns
 {
 	int ret = 0;
 
+#ifdef CONFIG_MMU
 	if (addr < dac_mmap_min_addr) {
 		ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
 				  SECURITY_CAP_AUDIT);
@@ -1012,5 +1013,6 @@ int cap_file_mmap(struct file *file, uns
 		if (ret == 0)
 			current->flags |= PF_SUPERPRIV;
 	}
+#endif
 	return ret;
 }
diff -puN security/selinux/hooks.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check security/selinux/hooks.c
--- a/security/selinux/hooks.c~nommu-ignore-the-address-parameter-in-the-file_mmap-security-check
+++ a/security/selinux/hooks.c
@@ -3046,6 +3046,7 @@ static int selinux_file_mmap(struct file
 			     unsigned long addr, unsigned long addr_only)
 {
 	int rc = 0;
+#ifdef CONFIG_MMU
 	u32 sid = current_sid();
 
 	/*
@@ -3060,6 +3061,7 @@ static int selinux_file_mmap(struct file
 		if (rc)
 			return rc;
 	}
+#endif
 
 	/* do DAC check on address space usage */
 	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
_

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Mike Frysinger]

On Tue, Nov 17, 2009 at 17:13, Andrew Morton wrote:
> On Fri, 16 Oct 2009 11:55:29 -0400 Eric Paris wrote:
>> On Fri, 2009-10-16 at 16:43 +0100, David Howells wrote:
>> > Eric Paris <eparis@redhat.com> wrote:
>> > > That would still call cap_file_mmap() and wouldn't solve your problem.
>> >
>> > Hmmm...  I guess I don't see the problem occur because I always run the
>> > programs as root.
>>
>> most likely yes, your processes are going to have CAP_SYS_RAWIO and will
>> happily sail through the cap_file_mmap().
>>
>> > I would guess that cap_file_mmap() and selinux_file_mmap() are, perhaps, too
>> > strict.  The hint shouldn't be rejected unless MAP_FIXED is also set, surely,
>> > but should rather be revised upwards.
>>
>> On the mmu side we don't check until the kernel has turned the hint into
>> a real address.  Which is apparently different then where the hook was
>> originally placed in the nommu side.
>>
>> > Certainly, addr==NULL and !MAP_FIXED is a reasonable case to permit, even in
>> > tightly secured MMU and SELinux mode...  After all, the manual page says:
>> >
>> >     If addr is NULL, then the kernel chooses the address at which to create
>> >     the mapping; this is the most portable method of creating  a  new  map-
>> >     ping.
>>
>> I agree, I think the choices are
>>
>> A) agree that we just shouldn't check the address on nommu
>>    A1) change kconfig to not allow the setting of these
>>       (easy to do for the LSM check, but not easy as it stands today
>>        for the cap_file_mmap() check)
>>    A2) change the addr_only to be a flags which indicate not to check
>>       (my original suggestion)
>>    A3) push the config_nommu down into the security code
>>       (your patch)
>> B) actually check the address, which requires moving the hook to a
>> location where it has been resolved.
>>       (graff yang's patch)
>>
>> I'll leave it up to you to determine which you like the best since you
>> know the implications of nommu.
>
> So what's the status of this issue now?  Should I merge the below into
> 2.6.32?

i think the patch as posted is the way to go, at least for now, and
should be merged.  it certainly works for us under Blackfin going by
Graff's tests.  but obviously this is ultimately David's call.
-mike

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Tue, 2009-11-17 at 14:13 -0800, Andrew Morton wrote:
> On Fri, 16 Oct 2009 11:55:29 -0400
> Eric Paris <eparis@redhat.com> wrote:
> 
> > On Fri, 2009-10-16 at 16:43 +0100, David Howells wrote:
> > > Eric Paris <eparis@redhat.com> wrote:
> > > 
> > > > That would still call cap_file_mmap() and wouldn't solve your problem.
> > > 
> > > Hmmm...  I guess I don't see the problem occur because I always run the
> > > programs as root.
> > 
> > most likely yes, your processes are going to have CAP_SYS_RAWIO and will
> > happily sail through the cap_file_mmap().
> > 
> > > I would guess that cap_file_mmap() and selinux_file_mmap() are, perhaps, too
> > > strict.  The hint shouldn't be rejected unless MAP_FIXED is also set, surely,
> > > but should rather be revised upwards.
> > 
> > On the mmu side we don't check until the kernel has turned the hint into
> > a real address.  Which is apparently different then where the hook was
> > originally placed in the nommu side.
> > 
> > > Certainly, addr==NULL and !MAP_FIXED is a reasonable case to permit, even in
> > > tightly secured MMU and SELinux mode...  After all, the manual page says:
> > > 
> > > 	If addr is NULL, then the kernel chooses the address at which to create
> > > 	the mapping; this is the most portable method of creating  a  new  map-
> > > 	ping.
> > 
> > I agree, I think the choices are
> > 
> > A) agree that we just shouldn't check the address on nommu
> >    A1) change kconfig to not allow the setting of these
> > 	(easy to do for the LSM check, but not easy as it stands today
> > 	 for the cap_file_mmap() check)
> >    A2) change the addr_only to be a flags which indicate not to check
> > 	(my original suggestion)
> >    A3) push the config_nommu down into the security code
> > 	(your patch)
> > B) actually check the address, which requires moving the hook to a
> > location where it has been resolved.
> > 	(graff yang's patch)
> > 
> > I'll leave it up to you to determine which you like the best since you
> > know the implications of nommu.
> > 
> 
> So what's the status of this issue now?  Should I merge the below into
> 2.6.32?
> 
> Thanks.

David??
  I think A2 is the right fix.  Apparmour is pushing to get in soon and
I know their implementation didn't check for CONFIG_MMU.  It's just too
easy for an LSM writer to get this wrong.  This patch is better than
what we have today, but I'll try to get something by the end of the week
which I think makes us all happy.

-Eric

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> So what's the status of this issue now?  Should I merge the below into
> 2.6.32?

Yes please.

Thanks,
David

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Andrew Morton]

On Fri, 20 Nov 2009 15:00:09 +0000 David Howells <dhowells@redhat.com> wrote:

> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > So what's the status of this issue now?  Should I merge the below into
> > 2.6.32?
> 
> Yes please.

I'll hold off, as Eric is preparing an alternative for "the end of this
week".  If that doesn't work out, we can add
nommu-ignore-the-address-parameter-in-the-file_mmap-security-check.patch
to 2.6.32.1, OK?

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> I'll hold off, as Eric is preparing an alternative for "the end of this
> week".  If that doesn't work out, we can add
> nommu-ignore-the-address-parameter-in-the-file_mmap-security-check.patch
> to 2.6.32.1, OK?

I'll be on holiday next week.

David

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Fri, 2009-11-20 at 17:54 +0000, David Howells wrote:
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > I'll hold off, as Eric is preparing an alternative for "the end of this
> > week".  If that doesn't work out, we can add
> > nommu-ignore-the-address-parameter-in-the-file_mmap-security-check.patch
> > to 2.6.32.1, OK?
> 
> I'll be on holiday next week.

Can we give this a whirl?  I can't even seem to make a config not select
MMU, so it isn't even compile tested in that case.  If it is good, I'll
send as a clean message for James Morris to take through the security
tree....

-Eric

---

commit 58c728c7f9c2c8e2c62f7dfda3e10f77524c4379
Author: Eric Paris <eparis@redhat.com>
Date:   Fri Nov 20 14:23:57 2009 -0500

    security: do not check mmap_min_addr on nommu systems
    
    nommu systems can do anything with memory they please and so they already
    win.  mmap_min_addr is the least of their worries.  Currently the
    mmap_min_addr implementation is problamatic on such systems.  This patch
    changes the addr_only argument to be a flags which can take the arguments
    for addr_only or not_addr.  LSMs then need to properly implement these two
    flags.
    
    Signed-off-by: Eric Paris <eparis@redhat.com>

diff --git a/include/linux/security.h b/include/linux/security.h
index 9c3a43b..a95ca48 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -43,6 +43,10 @@
 #define SECURITY_CAP_NOAUDIT 0
 #define SECURITY_CAP_AUDIT 1
 
+/* sec_flags for security_file_mmap */
+#define SECURITY_MMAP_ADDR_ONLY	0x01
+#define SECURITY_MMAP_NOT_ADDR	0x02
+
 struct ctl_table;
 struct audit_krule;
 
@@ -69,7 +73,7 @@ extern int cap_inode_need_killpriv(struct dentry *dentry);
 extern int cap_inode_killpriv(struct dentry *dentry);
 extern int cap_file_mmap(struct file *file, unsigned long reqprot,
 			 unsigned long prot, unsigned long flags,
-			 unsigned long addr, unsigned long addr_only);
+			 unsigned long addr, unsigned long sec_flags);
 extern int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags);
 extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			  unsigned long arg4, unsigned long arg5);
@@ -609,6 +613,8 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@reqprot contains the protection requested by the application.
  *	@prot contains the protection that will be applied by the kernel.
  *	@flags contains the operational flags.
+ *	@addr address vm will map to
+ *	@sec_flags what security checks should be done
  *	Return 0 if permission is granted.
  * @file_mprotect:
  *	Check permissions before changing memory access permissions.
@@ -1556,7 +1562,7 @@ struct security_operations {
 	int (*file_mmap) (struct file *file,
 			  unsigned long reqprot, unsigned long prot,
 			  unsigned long flags, unsigned long addr,
-			  unsigned long addr_only);
+			  unsigned long sec_flags);
 	int (*file_mprotect) (struct vm_area_struct *vma,
 			      unsigned long reqprot,
 			      unsigned long prot);
@@ -1826,7 +1832,7 @@ void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int security_file_mmap(struct file *file, unsigned long reqprot,
 			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only);
+			unsigned long addr, unsigned long sec_flags);
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			   unsigned long prot);
 int security_file_lock(struct file *file, unsigned int cmd);
@@ -2323,9 +2329,9 @@ static inline int security_file_mmap(struct file *file, unsigned long reqprot,
 				     unsigned long prot,
 				     unsigned long flags,
 				     unsigned long addr,
-				     unsigned long addr_only)
+				     unsigned long sec_flags)
 {
-	return cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
+	return cap_file_mmap(file, reqprot, prot, flags, addr, sec_flags);
 }
 
 static inline int security_file_mprotect(struct vm_area_struct *vma,
diff --git a/mm/mmap.c b/mm/mmap.c
index 828ecbf..fb7eb10 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1664,7 +1664,8 @@ static int expand_downwards(struct vm_area_struct *vma,
 		return -ENOMEM;
 
 	address &= PAGE_MASK;
-	error = security_file_mmap(NULL, 0, 0, 0, address, 1);
+	error = security_file_mmap(NULL, 0, 0, 0, address,
+				   SECURITY_MMAP_ADDR_ONLY);
 	if (error)
 		return error;
 
@@ -2005,7 +2006,8 @@ unsigned long do_brk(unsigned long addr, unsigned long len)
 	if (is_hugepage_only_range(mm, addr, len))
 		return -EINVAL;
 
-	error = security_file_mmap(NULL, 0, 0, 0, addr, 1);
+	error = security_file_mmap(NULL, 0, 0, 0, addr,
+				   SECURITY_MMAP_ADDR_ONLY);
 	if (error)
 		return error;
 
diff --git a/mm/mremap.c b/mm/mremap.c
index 97bff25..d308319 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -313,7 +313,8 @@ unsigned long do_mremap(unsigned long addr,
 		if ((addr <= new_addr) && (addr+old_len) > new_addr)
 			goto out;
 
-		ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+		ret = security_file_mmap(NULL, 0, 0, 0, new_addr,
+					 SECURITY_MMAP_ADDR_ONLY);
 		if (ret)
 			goto out;
 
@@ -421,7 +422,8 @@ unsigned long do_mremap(unsigned long addr,
 				goto out;
 			}
 
-			ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+			ret = security_file_mmap(NULL, 0, 0, 0, new_addr,
+						 SECURITY_MMAP_ADDR_ONLY);
 			if (ret)
 				goto out;
 		}
diff --git a/mm/nommu.c b/mm/nommu.c
index 9876fa0..df6fa1a 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -974,7 +974,8 @@ static int validate_mmap_request(struct file *file,
 	}
 
 	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	ret = security_file_mmap(file, reqprot, prot, flags, 0, 
+				 SECURITY_MMAP_NOT_ADDR);
 	if (ret < 0)
 		return ret;
 
diff --git a/security/commoncap.c b/security/commoncap.c
index 45b87af..6cf77c9 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -992,7 +992,7 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
  * @prot: unused
  * @flags: unused
  * @addr: address attempting to be mapped
- * @addr_only: unused
+ * @sec_flags: should the addr be checked?
  *
  * If the process is attempting to map memory below mmap_min_addr they need
  * CAP_SYS_RAWIO.  The other parameters to this function are unused by the
@@ -1001,11 +1001,12 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
  */
 int cap_file_mmap(struct file *file, unsigned long reqprot,
 		  unsigned long prot, unsigned long flags,
-		  unsigned long addr, unsigned long addr_only)
+		  unsigned long addr, unsigned long sec_flags)
 {
 	int ret = 0;
 
-	if (addr < dac_mmap_min_addr) {
+	if (!(sec_flags & SECURITY_MMAP_NOT_ADDR) &&
+	    (addr < dac_mmap_min_addr)) {
 		ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
 				  SECURITY_CAP_AUDIT);
 		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
diff --git a/security/security.c b/security/security.c
index b6e43a1..aa4e123 100644
--- a/security/security.c
+++ b/security/security.c
@@ -677,11 +677,11 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 int security_file_mmap(struct file *file, unsigned long reqprot,
 			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only)
+			unsigned long addr, unsigned long sec_flags)
 {
 	int ret;
 
-	ret = security_ops->file_mmap(file, reqprot, prot, flags, addr, addr_only);
+	ret = security_ops->file_mmap(file, reqprot, prot, flags, addr, sec_flags);
 	if (ret)
 		return ret;
 	return ima_file_mmap(file, prot);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 18e2e5b..93540af 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3043,7 +3043,7 @@ error:
 
 static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 			     unsigned long prot, unsigned long flags,
-			     unsigned long addr, unsigned long addr_only)
+			     unsigned long addr, unsigned long sec_flags)
 {
 	int rc = 0;
 	u32 sid = current_sid();
@@ -3054,7 +3054,8 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 	 * at bad behaviour/exploit that we always want to get the AVC, even
 	 * if DAC would have also denied the operation.
 	 */
-	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
+	if (!(sec_flags & SECURITY_MMAP_NOT_ADDR) &&
+	    (addr < CONFIG_LSM_MMAP_MIN_ADDR)) {
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
 		if (rc)
@@ -3062,8 +3063,8 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 	}
 
 	/* do DAC check on address space usage */
-	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (rc || addr_only)
+	rc = cap_file_mmap(file, reqprot, prot, flags, addr, sec_flags);
+	if (rc || (sec_flags & SECURITY_MMAP_ADDR_ONLY))
 		return rc;
 
 	if (selinux_checkreqprot)

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Andrew Morton]

On Fri, 20 Nov 2009 14:32:02 -0500
Eric Paris <eparis@redhat.com> wrote:

> On Fri, 2009-11-20 at 17:54 +0000, David Howells wrote:
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > I'll hold off, as Eric is preparing an alternative for "the end of this
> > > week".  If that doesn't work out, we can add
> > > nommu-ignore-the-address-parameter-in-the-file_mmap-security-check.patch
> > > to 2.6.32.1, OK?
> > 
> > I'll be on holiday next week.
> 
> Can we give this a whirl?  I can't even seem to make a config not select
> MMU, so it isn't even compile tested in that case.  If it is good, I'll
> send as a clean message for James Morris to take through the security
> tree....
> 

You'll need a cross-compiler.

> commit 58c728c7f9c2c8e2c62f7dfda3e10f77524c4379
> Author: Eric Paris <eparis@redhat.com>
> Date:   Fri Nov 20 14:23:57 2009 -0500
> 
>     security: do not check mmap_min_addr on nommu systems
>     
>     nommu systems can do anything with memory they please and so they already
>     win.  mmap_min_addr is the least of their worries.  Currently the
>     mmap_min_addr implementation is problamatic on such systems.  This patch
>     changes the addr_only argument to be a flags which can take the arguments
>     for addr_only or not_addr.  LSMs then need to properly implement these two
>     flags.
>     
>     Signed-off-by: Eric Paris <eparis@redhat.com>

Patch doesn't apply to current mainline for some reason.  I fixed that
up and checked that the affected files compile OK on superh.

The patch adds trailing whitespace.  If only we had a tool for that ;)

diff -puN include/linux/security.h~mm-nommuc-fix-improperly-call-of-security-api-in-mmap include/linux/security.h
--- a/include/linux/security.h~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/include/linux/security.h
@@ -43,6 +43,10 @@
 #define SECURITY_CAP_NOAUDIT 0
 #define SECURITY_CAP_AUDIT 1
 
+/* sec_flags for security_file_mmap */
+#define SECURITY_MMAP_ADDR_ONLY	0x01
+#define SECURITY_MMAP_NOT_ADDR	0x02
+
 struct ctl_table;
 struct audit_krule;
 
@@ -69,7 +73,7 @@ extern int cap_inode_need_killpriv(struc
 extern int cap_inode_killpriv(struct dentry *dentry);
 extern int cap_file_mmap(struct file *file, unsigned long reqprot,
 			 unsigned long prot, unsigned long flags,
-			 unsigned long addr, unsigned long addr_only);
+			 unsigned long addr, unsigned long sec_flags);
 extern int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags);
 extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			  unsigned long arg4, unsigned long arg5);
@@ -593,6 +597,8 @@ static inline void security_free_mnt_opt
  *	@reqprot contains the protection requested by the application.
  *	@prot contains the protection that will be applied by the kernel.
  *	@flags contains the operational flags.
+ *	@addr address vm will map to
+ *	@sec_flags what security checks should be done
  *	Return 0 if permission is granted.
  * @file_mprotect:
  *	Check permissions before changing memory access permissions.
@@ -1535,7 +1541,7 @@ struct security_operations {
 	int (*file_mmap) (struct file *file,
 			  unsigned long reqprot, unsigned long prot,
 			  unsigned long flags, unsigned long addr,
-			  unsigned long addr_only);
+			  unsigned long sec_flags);
 	int (*file_mprotect) (struct vm_area_struct *vma,
 			      unsigned long reqprot,
 			      unsigned long prot);
@@ -1804,7 +1810,7 @@ void security_file_free(struct file *fil
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int security_file_mmap(struct file *file, unsigned long reqprot,
 			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only);
+			unsigned long addr, unsigned long sec_flags);
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			   unsigned long prot);
 int security_file_lock(struct file *file, unsigned int cmd);
@@ -2300,9 +2306,9 @@ static inline int security_file_mmap(str
 				     unsigned long prot,
 				     unsigned long flags,
 				     unsigned long addr,
-				     unsigned long addr_only)
+				     unsigned long sec_flags)
 {
-	return cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
+	return cap_file_mmap(file, reqprot, prot, flags, addr, sec_flags);
 }
 
 static inline int security_file_mprotect(struct vm_area_struct *vma,
diff -puN mm/mmap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap mm/mmap.c
--- a/mm/mmap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/mm/mmap.c
@@ -1668,7 +1668,8 @@ static int expand_downwards(struct vm_ar
 		return -ENOMEM;
 
 	address &= PAGE_MASK;
-	error = security_file_mmap(NULL, 0, 0, 0, address, 1);
+	error = security_file_mmap(NULL, 0, 0, 0, address,
+				   SECURITY_MMAP_ADDR_ONLY);
 	if (error)
 		return error;
 
@@ -2009,7 +2010,8 @@ unsigned long do_brk(unsigned long addr,
 	if (is_hugepage_only_range(mm, addr, len))
 		return -EINVAL;
 
-	error = security_file_mmap(NULL, 0, 0, 0, addr, 1);
+	error = security_file_mmap(NULL, 0, 0, 0, addr,
+				   SECURITY_MMAP_ADDR_ONLY);
 	if (error)
 		return error;
 
diff -puN mm/mremap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap mm/mremap.c
--- a/mm/mremap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/mm/mremap.c
@@ -313,7 +313,8 @@ unsigned long do_mremap(unsigned long ad
 		if ((addr <= new_addr) && (addr+old_len) > new_addr)
 			goto out;
 
-		ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+		ret = security_file_mmap(NULL, 0, 0, 0, new_addr,
+					 SECURITY_MMAP_ADDR_ONLY);
 		if (ret)
 			goto out;
 
@@ -421,7 +422,8 @@ unsigned long do_mremap(unsigned long ad
 				goto out;
 			}
 
-			ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+			ret = security_file_mmap(NULL, 0, 0, 0, new_addr,
+						 SECURITY_MMAP_ADDR_ONLY);
 			if (ret)
 				goto out;
 		}
diff -puN mm/nommu.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap mm/nommu.c
--- a/mm/nommu.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/mm/nommu.c
@@ -974,7 +974,8 @@ static int validate_mmap_request(struct 
 	}
 
 	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	ret = security_file_mmap(file, reqprot, prot, flags, 0, 
+				 SECURITY_MMAP_NOT_ADDR);
 	if (ret < 0)
 		return ret;
 
diff -puN security/commoncap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap security/commoncap.c
--- a/security/commoncap.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/security/commoncap.c
@@ -992,7 +992,7 @@ int cap_vm_enough_memory(struct mm_struc
  * @prot: unused
  * @flags: unused
  * @addr: address attempting to be mapped
- * @addr_only: unused
+ * @sec_flags: should the addr be checked?
  *
  * If the process is attempting to map memory below mmap_min_addr they need
  * CAP_SYS_RAWIO.  The other parameters to this function are unused by the
@@ -1001,11 +1001,12 @@ int cap_vm_enough_memory(struct mm_struc
  */
 int cap_file_mmap(struct file *file, unsigned long reqprot,
 		  unsigned long prot, unsigned long flags,
-		  unsigned long addr, unsigned long addr_only)
+		  unsigned long addr, unsigned long sec_flags)
 {
 	int ret = 0;
 
-	if (addr < dac_mmap_min_addr) {
+	if (!(sec_flags & SECURITY_MMAP_NOT_ADDR) &&
+	    (addr < dac_mmap_min_addr)) {
 		ret = cap_capable(current, current_cred(), CAP_SYS_RAWIO,
 				  SECURITY_CAP_AUDIT);
 		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
diff -puN security/security.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap security/security.c
--- a/security/security.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/security/security.c
@@ -637,9 +637,9 @@ int security_file_ioctl(struct file *fil
 
 int security_file_mmap(struct file *file, unsigned long reqprot,
 			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only)
+			unsigned long addr, unsigned long sec_flags)
 {
-	return security_ops->file_mmap(file, reqprot, prot, flags, addr, addr_only);
+	return security_ops->file_mmap(file, reqprot, prot, flags, addr, sec_flags);
 }
 
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
diff -puN security/selinux/hooks.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap security/selinux/hooks.c
--- a/security/selinux/hooks.c~mm-nommuc-fix-improperly-call-of-security-api-in-mmap
+++ a/security/selinux/hooks.c
@@ -3043,7 +3043,7 @@ error:
 
 static int selinux_file_mmap(struct file *file, unsigned long reqprot,
 			     unsigned long prot, unsigned long flags,
-			     unsigned long addr, unsigned long addr_only)
+			     unsigned long addr, unsigned long sec_flags)
 {
 	int rc = 0;
 	u32 sid = current_sid();
@@ -3054,7 +3054,8 @@ static int selinux_file_mmap(struct file
 	 * at bad behaviour/exploit that we always want to get the AVC, even
 	 * if DAC would have also denied the operation.
 	 */
-	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
+	if (!(sec_flags & SECURITY_MMAP_NOT_ADDR) &&
+	    (addr < CONFIG_LSM_MMAP_MIN_ADDR)) {
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
 		if (rc)
@@ -3062,8 +3063,8 @@ static int selinux_file_mmap(struct file
 	}
 
 	/* do DAC check on address space usage */
-	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (rc || addr_only)
+	rc = cap_file_mmap(file, reqprot, prot, flags, addr, sec_flags);
+	if (rc || (sec_flags & SECURITY_MMAP_ADDR_ONLY))
 		return rc;
 
 	if (selinux_checkreqprot)
_

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Fri, 2009-11-20 at 11:50 -0800, Andrew Morton wrote:
> On Fri, 20 Nov 2009 14:32:02 -0500

> >     security: do not check mmap_min_addr on nommu systems
> >     
> >     nommu systems can do anything with memory they please and so they already
> >     win.  mmap_min_addr is the least of their worries.  Currently the
> >     mmap_min_addr implementation is problamatic on such systems.  This patch
> >     changes the addr_only argument to be a flags which can take the arguments
> >     for addr_only or not_addr.  LSMs then need to properly implement these two
> >     flags.
> >     
> >     Signed-off-by: Eric Paris <eparis@redhat.com>
> 
> Patch doesn't apply to current mainline for some reason.  I fixed that
> up and checked that the affected files compile OK on superh.

It was against my linux-next devel tree, probably something new in
James' for-next tree.  Are people hoping for this in stable or should we
just queue for -next?  This is a pretty old bug I guess people are just
hitting for the first time...

> The patch adds trailing whitespace.  If only we had a tool for that ;)

Fixed.

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[David Howells]

Eric Paris <eparis@redhat.com> wrote:

> +/* sec_flags for security_file_mmap */
> +#define SECURITY_MMAP_ADDR_ONLY	0x01
> +#define SECURITY_MMAP_NOT_ADDR	0x02

Please add comments to these to indicate what they're intended to convey.
Would ADDR_ONLY be better as EXACT_ADDR?

David

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[Eric Paris]

On Sat, 2009-11-21 at 00:16 +0000, David Howells wrote:
> Eric Paris <eparis@redhat.com> wrote:
> 
> > +/* sec_flags for security_file_mmap */
> > +#define SECURITY_MMAP_ADDR_ONLY	0x01
> > +#define SECURITY_MMAP_NOT_ADDR	0x02
> 
> Please add comments to these to indicate what they're intended to convey.
> Would ADDR_ONLY be better as EXACT_ADDR?

I think I should point out that this hook checks 2 things.  Originally
it was only used to check if a file should be allowed to be mmaped.  It
was later enhanced to check if the return address of mmap, if it is file
backed or anonymous, is acceptable.  These flags only influence the
later.

ADDR_ONLY means the security system should only check the address.
NOT_ADDR means they security system should not check the address.

You need ADDR_ONLY when the hook is called on map that is not file
backed or where that has already been dealt with.  You need NOT_ADDR
only for nommu where the whole idea of mmap_min_addr is pointless.

I'm not sure what comments would convey....

/* security hook should only check the address */
#define SECURITY_MMAP_ADDR_ONLY	0x01
/* security hook should not check the address */
#define SECURITY_MMAP_NOT_ADDR	0x02

Does that add something?

Still haven't heard where people scream they absolutely need this today,
so I'm going to ask James to carry it in his for-next tree.

Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

[John Johansen]

Eric Paris wrote:
> On Sat, 2009-11-21 at 00:16 +0000, David Howells wrote:
>> Eric Paris <eparis@redhat.com> wrote:
>>
>>> +/* sec_flags for security_file_mmap */
>>> +#define SECURITY_MMAP_ADDR_ONLY	0x01
>>> +#define SECURITY_MMAP_NOT_ADDR	0x02
>> Please add comments to these to indicate what they're intended to convey.
>> Would ADDR_ONLY be better as EXACT_ADDR?
> 
> I think I should point out that this hook checks 2 things.  Originally
> it was only used to check if a file should be allowed to be mmaped.  It
> was later enhanced to check if the return address of mmap, if it is file
> backed or anonymous, is acceptable.  These flags only influence the
> later.
> 
> ADDR_ONLY means the security system should only check the address.
> NOT_ADDR means they security system should not check the address.
> 
> You need ADDR_ONLY when the hook is called on map that is not file
> backed or where that has already been dealt with.  You need NOT_ADDR
> only for nommu where the whole idea of mmap_min_addr is pointless.
> 
> I'm not sure what comments would convey....
> 
> /* security hook should only check the address */
> #define SECURITY_MMAP_ADDR_ONLY	0x01
> /* security hook should not check the address */
> #define SECURITY_MMAP_NOT_ADDR	0x02
> 
> Does that add something?
> 
> Still haven't heard where people scream they absolutely need this today,
> so I'm going to ask James to carry it in his for-next tree.
> 
The comments convey a tad more but I don't think they are necessary, and
I concur, it would be good if it went into the for-next tree.