[PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Roland McGrath]

An mmap with PROT_NONE is done specifically to ensure that an address will
fault.  So doing this on addresses below mmap_min_addr is not seeking a
"dangerous" operation.  Conversely, it's an attempt to ensure robustness in
case mmap_min_addr is less restrictive than the user wants to be.

Since we might let a low mapping exist at all without a check, we add
another check to prevent mprotect from granting access to such a mapping.

Signed-off-by: Roland McGrath <roland@hack.frob.com>
---
 include/linux/security.h |    5 ++-
 security/apparmor/lsm.c  |    7 ++++++
 security/capability.c    |    6 -----
 security/commoncap.c     |   48 ++++++++++++++++++++++++++++++++++++---------
 security/selinux/hooks.c |    8 ++++++-
 5 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index ebd2a53..aba8071 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -73,6 +73,8 @@ 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);
+extern int cap_file_mprotect(struct vm_area_struct *vma,
+			     unsigned long reqprot, unsigned long prot);
 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);
@@ -2213,7 +2215,7 @@ static inline int security_file_mprotect(struct vm_area_struct *vma,
 					 unsigned long reqprot,
 					 unsigned long prot)
 {
-	return 0;
+	return cap_file_mprotect(vma, reqprot, prot);
 }
 
 static inline int security_file_lock(struct file *file, unsigned int cmd)
@@ -3044,4 +3046,3 @@ static inline void free_secdata(void *secdata)
 #endif /* CONFIG_SECURITY */
 
 #endif /* ! __LINUX_SECURITY_H */
-
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 3783202..d2a9693 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -508,6 +508,13 @@ static int apparmor_file_mmap(struct file *file, unsigned long reqprot,
 static int apparmor_file_mprotect(struct vm_area_struct *vma,
 				  unsigned long reqprot, unsigned long prot)
 {
+        int rc;
+
+	/* do DAC check */
+	rc = cap_file_mprotect(vma, reqprot, prot);
+	if (rc)
+		return rc;
+
 	return common_mmap(OP_FMPROT, vma->vm_file, prot,
 			   !(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0);
 }
diff --git a/security/capability.c b/security/capability.c
index 2984ea4..3c60f07 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -316,12 +316,6 @@ static int cap_file_ioctl(struct file *file, unsigned int command,
 	return 0;
 }
 
-static int cap_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
-			     unsigned long prot)
-{
-	return 0;
-}
-
 static int cap_file_lock(struct file *file, unsigned int cmd)
 {
 	return 0;
diff --git a/security/commoncap.c b/security/commoncap.c
index a93b3b7..0d4685a 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -942,11 +942,26 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
 
+static int cap_mmap_min_addr(unsigned long addr)
+{
+	int ret = 0;
+
+	if (addr < dac_mmap_min_addr) {
+		ret = cap_capable(current, current_cred(), &init_user_ns,
+				  CAP_SYS_RAWIO, SECURITY_CAP_AUDIT);
+		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
+		if (ret == 0)
+			current->flags |= PF_SUPERPRIV;
+	}
+
+	return ret;
+}
+
 /*
  * cap_file_mmap - check if able to map given addr
  * @file: unused
  * @reqprot: unused
- * @prot: unused
+ * @prot: protection being requested
  * @flags: unused
  * @addr: address attempting to be mapped
  * @addr_only: unused
@@ -960,14 +975,27 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
 		  unsigned long prot, unsigned long flags,
 		  unsigned long addr, unsigned long addr_only)
 {
-	int ret = 0;
+	if (addr_only || prot != PROT_NONE)
+		return cap_mmap_min_addr(addr);
+	return 0;
+}
 
-	if (addr < dac_mmap_min_addr) {
-		ret = cap_capable(current, current_cred(), &init_user_ns, CAP_SYS_RAWIO,
-				  SECURITY_CAP_AUDIT);
-		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
-		if (ret == 0)
-			current->flags |= PF_SUPERPRIV;
-	}
-	return ret;
+/*
+ * cap_file_mprotect - check if able to mprotect given addr
+ * @vma: entry being changed
+ * @reqprot: unused
+ * @prot: protection being changed to
+ *
+ * If the process is attempting to change memory below dac_mmap_min_addr to
+ * anything but PROT_NONE, they need CAP_SYS_RAWIO.  The other parameters
+ * to this function are unused by the capability security module.  Returns
+ * 0 if this mapping should be allowed -EPERM if not.
+ */
+int cap_file_mprotect(struct vm_area_struct *vma,
+		      unsigned long reqprot,
+		      unsigned long prot)
+{
+	if (prot != PROT_NONE)
+		return cap_mmap_min_addr(vma->vm_start);
+	return 0;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 266a229..76e6f04 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3086,13 +3086,19 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
 				 unsigned long prot)
 {
 	const struct cred *cred = current_cred();
+	int rc;
 
 	if (selinux_checkreqprot)
 		prot = reqprot;
 
+	/* do DAC check on address space usage */
+	rc = cap_file_mprotect(vma, reqprot, prot)
+	if (rc)
+		return rc;
+
 	if (default_noexec &&
 	    (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
-		int rc = 0;
+		rc = 0;
 		if (vma->vm_start >= vma->vm_mm->start_brk &&
 		    vma->vm_end <= vma->vm_mm->brk) {
 			rc = cred_has_perm(cred, cred, PROCESS__EXECHEAP);

[PATCH 2/2] SELinux: Do not apply MMAP_ZERO check to PROT_NONE mappings

[Roland McGrath]

An mmap with PROT_NONE is done specifically to ensure that an address
will fault.  So doing this on addresses below CONFIG_LSM_MMAP_MIN_ADDR
is not seeking a "dangerous" operation.  Conversely, it's an attempt
to ensure robustness in case CONFIG_LSM_MMAP_MIN_ADDR or vm.mmap_min_addr
is less restrictive than the user wants to be.

Since we might let a low mapping exist at all without a check, we
add another check to prevent mprotect from granting access to such
a mapping without passing an MMAP_ZERO security check.

Signed-off-by: Roland McGrath <roland@hack.frob.com>
---
 security/selinux/hooks.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 76e6f04..1e3657b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3062,7 +3062,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 (addr < CONFIG_LSM_MMAP_MIN_ADDR &&
+	    (addr_only || prot != PROT_NONE)) {
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
 		if (rc)
@@ -3091,6 +3092,20 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
 	if (selinux_checkreqprot)
 		prot = reqprot;
 
+	/*
+	 * Notice that we are intentionally putting the SELinux check before
+	 * the secondary cap_file_mprotect check.  This is such a likely attempt
+	 * 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 && prot != PROT_NONE) {
+		u32 sid = current_sid();
+		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
+				  MEMPROTECT__MMAP_ZERO, NULL);
+		if (rc)
+			return rc;
+	}
+
 	/* do DAC check on address space usage */
 	rc = cap_file_mprotect(vma, reqprot, prot)
 	if (rc)

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Linus Torvalds]

So I'm not against this, but I'm wondering what triggers the need for it?

It does make the security checks more complicated, since now
mprotect() suddenly has to care about mmap_min_addr. So I don't think
it's a security enhancement ("attempt to ensure robustness").

But if there is some actual use-case that is shown to be helped,
please document that n the explanations for the changeset.

                  Linus

On Sat, Oct 22, 2011 at 12:39 AM, Roland McGrath <roland@hack.frob.com> wrote:
> An mmap with PROT_NONE is done specifically to ensure that an address will
> fault.  So doing this on addresses below mmap_min_addr is not seeking a
> "dangerous" operation.  Conversely, it's an attempt to ensure robustness in
> case mmap_min_addr is less restrictive than the user wants to be.
>
> Since we might let a low mapping exist at all without a check, we add
> another check to prevent mprotect from granting access to such a mapping.
>
> Signed-off-by: Roland McGrath <roland@hack.frob.com>
> ---
>  include/linux/security.h |    5 ++-
>  security/apparmor/lsm.c  |    7 ++++++
>  security/capability.c    |    6 -----
>  security/commoncap.c     |   48 ++++++++++++++++++++++++++++++++++++---------
>  security/selinux/hooks.c |    8 ++++++-
>  5 files changed, 55 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ebd2a53..aba8071 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -73,6 +73,8 @@ 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);
> +extern int cap_file_mprotect(struct vm_area_struct *vma,
> +                            unsigned long reqprot, unsigned long prot);
>  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);
> @@ -2213,7 +2215,7 @@ static inline int security_file_mprotect(struct vm_area_struct *vma,
>                                         unsigned long reqprot,
>                                         unsigned long prot)
>  {
> -       return 0;
> +       return cap_file_mprotect(vma, reqprot, prot);
>  }
>
>  static inline int security_file_lock(struct file *file, unsigned int cmd)
> @@ -3044,4 +3046,3 @@ static inline void free_secdata(void *secdata)
>  #endif /* CONFIG_SECURITY */
>
>  #endif /* ! __LINUX_SECURITY_H */
> -
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 3783202..d2a9693 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -508,6 +508,13 @@ static int apparmor_file_mmap(struct file *file, unsigned long reqprot,
>  static int apparmor_file_mprotect(struct vm_area_struct *vma,
>                                  unsigned long reqprot, unsigned long prot)
>  {
> +        int rc;
> +
> +       /* do DAC check */
> +       rc = cap_file_mprotect(vma, reqprot, prot);
> +       if (rc)
> +               return rc;
> +
>        return common_mmap(OP_FMPROT, vma->vm_file, prot,
>                           !(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0);
>  }
> diff --git a/security/capability.c b/security/capability.c
> index 2984ea4..3c60f07 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -316,12 +316,6 @@ static int cap_file_ioctl(struct file *file, unsigned int command,
>        return 0;
>  }
>
> -static int cap_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> -                            unsigned long prot)
> -{
> -       return 0;
> -}
> -
>  static int cap_file_lock(struct file *file, unsigned int cmd)
>  {
>        return 0;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index a93b3b7..0d4685a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -942,11 +942,26 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
>        return __vm_enough_memory(mm, pages, cap_sys_admin);
>  }
>
> +static int cap_mmap_min_addr(unsigned long addr)
> +{
> +       int ret = 0;
> +
> +       if (addr < dac_mmap_min_addr) {
> +               ret = cap_capable(current, current_cred(), &init_user_ns,
> +                                 CAP_SYS_RAWIO, SECURITY_CAP_AUDIT);
> +               /* set PF_SUPERPRIV if it turns out we allow the low mmap */
> +               if (ret == 0)
> +                       current->flags |= PF_SUPERPRIV;
> +       }
> +
> +       return ret;
> +}
> +
>  /*
>  * cap_file_mmap - check if able to map given addr
>  * @file: unused
>  * @reqprot: unused
> - * @prot: unused
> + * @prot: protection being requested
>  * @flags: unused
>  * @addr: address attempting to be mapped
>  * @addr_only: unused
> @@ -960,14 +975,27 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
>                  unsigned long prot, unsigned long flags,
>                  unsigned long addr, unsigned long addr_only)
>  {
> -       int ret = 0;
> +       if (addr_only || prot != PROT_NONE)
> +               return cap_mmap_min_addr(addr);
> +       return 0;
> +}
>
> -       if (addr < dac_mmap_min_addr) {
> -               ret = cap_capable(current, current_cred(), &init_user_ns, CAP_SYS_RAWIO,
> -                                 SECURITY_CAP_AUDIT);
> -               /* set PF_SUPERPRIV if it turns out we allow the low mmap */
> -               if (ret == 0)
> -                       current->flags |= PF_SUPERPRIV;
> -       }
> -       return ret;
> +/*
> + * cap_file_mprotect - check if able to mprotect given addr
> + * @vma: entry being changed
> + * @reqprot: unused
> + * @prot: protection being changed to
> + *
> + * If the process is attempting to change memory below dac_mmap_min_addr to
> + * anything but PROT_NONE, they need CAP_SYS_RAWIO.  The other parameters
> + * to this function are unused by the capability security module.  Returns
> + * 0 if this mapping should be allowed -EPERM if not.
> + */
> +int cap_file_mprotect(struct vm_area_struct *vma,
> +                     unsigned long reqprot,
> +                     unsigned long prot)
> +{
> +       if (prot != PROT_NONE)
> +               return cap_mmap_min_addr(vma->vm_start);
> +       return 0;
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 266a229..76e6f04 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3086,13 +3086,19 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
>                                 unsigned long prot)
>  {
>        const struct cred *cred = current_cred();
> +       int rc;
>
>        if (selinux_checkreqprot)
>                prot = reqprot;
>
> +       /* do DAC check on address space usage */
> +       rc = cap_file_mprotect(vma, reqprot, prot)
> +       if (rc)
> +               return rc;
> +
>        if (default_noexec &&
>            (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
> -               int rc = 0;
> +               rc = 0;
>                if (vma->vm_start >= vma->vm_mm->start_brk &&
>                    vma->vm_end <= vma->vm_mm->brk) {
>                        rc = cred_has_perm(cred, cred, PROCESS__EXECHEAP);
>

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Roland McGrath]

> So I'm not against this, but I'm wondering what triggers the need for it?
> 
> It does make the security checks more complicated, since now
> mprotect() suddenly has to care about mmap_min_addr. So I don't think
> it's a security enhancement ("attempt to ensure robustness").
> 
> But if there is some actual use-case that is shown to be helped,
> please document that n the explanations for the changeset.

It's exactly the case that I did mention: an application's own attempt to
ensure robustness by doing a PROT_NONE mmap of the [0,0x10000) region.  An
application cannot presume that this region is already precluded from being
used by any non-MAP_FIXED mmap across all systems and configurations, so
it's defensive coding to explicitly block it off with a PROT_NONE mapping.

Since we know mmap_min_addr-type constraints might exist, we start at 0 and
move up a page as long as mmap fails.  That works fine to cover e.g. the
[0x1000,0x10000) region when mmap_min_addr is set to 4096, as is common.
However, under SELinux these harmless attempts are diagnosed as MMAP_ZERO
avc denials, which percolate up to the user as scare warnings that the
application was prevented from doing something dangerous and possibly
malicious, when that's not the case at all.


Thanks,
Roland

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Linus Torvalds]

On Sat, Oct 22, 2011 at 8:24 PM, Roland McGrath <roland@hack.frob.com> wrote:
>
> It's exactly the case that I did mention: an application's own attempt to
> ensure robustness by doing a PROT_NONE mmap of the [0,0x10000) region.  An
> application cannot presume that this region is already precluded from being
> used by any non-MAP_FIXED mmap across all systems and configurations, so
> it's defensive coding to explicitly block it off with a PROT_NONE mapping.

Quite frankly, I don't think that's an interesting case.

The app can try to be robust by doing the mmap(PROT_NONE), but if that
fails, then the app should just let it go.

Why should the kernel make its own security more complex (and thus fragile)?

So I think that it's perfectly ok for some user-level app to do

    mmap(NULL, 64k, PROT_NONE, MAP_FIXED|MAP_ANON, -1, 0);

to try to create a PROT_NONE mapping at NULL, but then just ignore the
error if that fails. That way the app can handle the case of "maybe
the system is set up to allow NULL mappings, so I'll harden it
myself".

But that's no reason for the kernel to *allow* the mapping.

And if the app actually *fails* due to the mmap failing, then the app
is just buggy and stupid. So again, there's no reason for the kernel
to allow it.

IOW, either the app is fine with the kernel not letting it do a
PROT_NONE mapping, or the app is so braindamaged as to not be worth
worrying about. In neither case do I actually see any reason to apply
your patch.

So I was looking for some *other* reason for the patch.

Because, quite frankly, "security hardening" is absolutely *not* a
reason to do it - complex security is not "hardened", it's just
"harder and more likely to be buggy".

                  Linus

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Linus Torvalds]

On Sat, Oct 22, 2011 at 9:32 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I was looking for some *other* reason for the patch.
>
> Because, quite frankly, "security hardening" is absolutely *not* a
> reason to do it - complex security is not "hardened", it's just
> "harder and more likely to be buggy".

Btw, if the only concern is "you don't want to elevate the selinux
denial to be some user-visible thing", then I'd suggest attacking
*that* issue directly.

For example, maybe we could fail the PROT_NONE mmap (ie not actually
create any mapping at all, and certainly not create anything that is
then mprotectable), but return success and not elevate it to be
reported.

But then it really is important to return success, because otherwise
it would be a "silent probe" of the security model (ie a bad user
could use the mmap(PROT_NONE) to see if min_mmap_addr is on or not
without triggering any selinux warnings).

And unlike your patch, it wouldn't open up some new interface
(mprotect) to worry about.

So I think it might be valid to say "always allow mmap(PROT_NONE)
under mmap_min_limit, by simply turning it into a no-op".

                       Linus

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Kevin Easton]

Quoting Linus Torvalds <torvalds@linux-foundation.org>:

> [ Resent, this seems to have gotten dropped by something. Sorry if it
> shows up twice  ]

My fault, looks like lkml.org trims the CC list to an unreasonably small
value.

> On Sun, Oct 23, 2011 at 2:08 AM, Kevin Easton <kevin@guarana.org> wrote:
>>
>> Won't this still allow silent probing, because the malicious user can
>> just try to create the mapping, then check in /proc/self/maps to see
>> if it really worked?
>
> Yup, right you are.
>
> So we shouldn't do that either, and probably just leave the current
> semantics, unless Roland (or others) can convince me that complicating
> the kernel  mmap security model really is worth it.
>
>             Linus

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Roland McGrath]

> The app can try to be robust by doing the mmap(PROT_NONE), but if that
> fails, then the app should just let it go.

Indeed the application does not get unhappy about the mmap failing.  It
does need to try it in a loop, moving up a page after each failure, though.
Its own robustness requirement mandates that space up to 0x10000 be blocked
off, even if the system is only enforcing it up to 0x1000, for example.

> But that's no reason for the kernel to *allow* the mapping.

I don't have a problem with that.

> So I was looking for some *other* reason for the patch.

As I explained, the rationale was not to have any LSM modules (such as
SELinux) complaining about the attempt.

> Btw, if the only concern is "you don't want to elevate the selinux
> denial to be some user-visible thing", then I'd suggest attacking
> *that* issue directly.

That's fine with me too.

> For example, maybe we could fail the PROT_NONE mmap (ie not actually
> create any mapping at all, and certainly not create anything that is
> then mprotectable), but return success and not elevate it to be
> reported.

That works, though AFAICT it needs this slightly more complex logic
repeated in each different LSM.

> But then it really is important to return success, because otherwise
> it would be a "silent probe" of the security model (ie a bad user
> could use the mmap(PROT_NONE) to see if min_mmap_addr is on or not
> without triggering any selinux warnings).

Agreed.

> And unlike your patch, it wouldn't open up some new interface
> (mprotect) to worry about.

I agree that is better.  What's important is that the application can rely
on the low memory range never being made accessible by any mmap.

> So I think it might be valid to say "always allow mmap(PROT_NONE)
> under mmap_min_limit, by simply turning it into a no-op".

Note that there is also CONFIG_LSM_MMAP_MIN_ADDR to consider, but yes.

> On Sun, Oct 23, 2011 at 2:08 AM, Kevin Easton <kevin@guarana.org> wrote:
> >
> > Won't this still allow silent probing, because the malicious user can
> > just try to create the mapping, then check in /proc/self/maps to see
> > if it really worked?
> 
> Yup, right you are.

Not if the application is already running inside a sandbox that prevents
direct access to /proc (e.g. in a private FS namespace, as my application
in fact is).

> So we shouldn't do that either, and probably just leave the current
> semantics, unless Roland (or others) can convince me that complicating
> the kernel  mmap security model really is worth it.

I'm really only concerned with two things:

1. The application can do something after which it can rely, with maximal
   paranoia, on the [0,0x10000) range never being used by any later
   non-MAP_FIXED mmap, regardless of kernel variant or local system
   configuration.
2. Whatever LSM stuff might be in use distinguishes the actions that ensure
   this from a "potentially malicious" attempt to do a usable mapping in
   that range.

So either the "pretend to succeed" or the "fail without LSM whining" models
can fit those needs as well as the "really succeed for PROT_NONE only"
model I proposed.  (Though I agree with your point above that "pretend to
succeed" is a little bit better because of potentially revealing less about
the exact constraints the system is imposing.)  In the status quo today,
AIUI the only way to do "fail without whining" under SELinux is with a
"dontaudit" policy rule for the MMAP_ZERO permission check for the given
application--but that doesn't distinguish the harmless PROT_NONE attempts
from an actual "malicious" attempt, so it's suboptimal.  (And I don't know
about other LSM implementations that might do their own kinds of whining.)

So it may be sufficient just to change SELinux (and perhaps other LSM
implementations) to treat PROT_NONE specially for purposes of the whining.
LSM folks will have to say how best to make that happen.


Thanks,
Roland

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Eric Paris]

On Sun, 2011-10-23 at 11:52 -0700, Roland McGrath wrote:

> > But that's no reason for the kernel to *allow* the mapping.
> 
> I don't have a problem with that.

I feel like, and it's just a very vague feeling, that the PROT bits
didn't matter to the kernel.  It would still happily execute stuff on
page 0 even without PROT_EXEC at some point in the past.  I'm probably
totally off base, and I could test it, but I sort of feel like I
remember something like that....

If that's the case, NULL pointer kernel bugs won't be caught if they
happen while these are mapped by your program...


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Eric Paris]

On Sun, 2011-10-23 at 11:52 -0700, Roland McGrath wrote:

> > But that's no reason for the kernel to *allow* the mapping.
> 
> I don't have a problem with that.

I feel like, and it's just a very vague feeling, that the PROT bits
didn't matter to the kernel.  It would still happily execute stuff on
page 0 even without PROT_EXEC at some point in the past.  I'm probably
totally off base, and I could test it, but I sort of feel like I
remember something like that....

If that's the case, NULL pointer kernel bugs won't be caught if they
happen while these are mapped by your program...

Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings

[Roland McGrath]

> I feel like, and it's just a very vague feeling, that the PROT bits
> didn't matter to the kernel.  It would still happily execute stuff on
> page 0 even without PROT_EXEC at some point in the past.  I'm probably
> totally off base, and I could test it, but I sort of feel like I
> remember something like that....

Saying that PROT_EXEC might not be enforced is quite a different thing than
saying "PROT bits don't matter".  It's certainly the case that in some past
kernel versions, some hardware (older x86 chips), some configurations (x86
non-PAE), and some modes (READ_IMPLIES_EXEC personality stuff), what you
can read, you can execute.  I sincerely doubt it's ever been the case that
anything mapped as PROT_NONE could be accessed in any manner.


Thanks,
Roland