| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 22 Oct 2011 06:38:33 +0300
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Roland McGrath <roland@...k.frob.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
James Morris <jmorris@...ei.org>,
Eric Paris <eparis@...isplace.org>,
Stephen Smalley <sds@...ho.nsa.gov>, selinux@...ho.nsa.gov,
John Johansen <john.johansen@...onical.com>,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] LSM: Do not apply mmap_min_addr check to PROT_NONE mappings
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@...k.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@...k.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);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists