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

[graff yang <graff.yang@gmail.com>]

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