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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 15 Oct 2009 11:45:07 +0800
From:	graff yang <graff.yang@...il.com>
To:	David Howells <dhowells@...hat.com>
Cc:	linux-kernel@...r.kernel.org, gyang@...ckfin.uclinux.org,
	akpm@...ux-foundation.org, uclinux-dist-devel@...ckfin.uclinux.org,
	Graff Yang <graf.yang@...log.com>,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH] mm/nommu.c: Fix improperly call of security API in mmap

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@...il.com> wrote:
> Hi, David,
> Thanks your patch, I will test it.
>
>
> On Wed, Oct 14, 2009 at 10:08 PM, David Howells <dhowells@...hat.com> wrote:
>> <graff.yang@...il.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@...hat.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@...hat.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
--
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/