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@xxxxxxxxx> wrote:
> Hi, David,
> Thanks your patch, I will test it.
>
>
> On Wed, Oct 14, 2009 at 10:08 PM, David Howells <dhowells@xxxxxxxxxx> wrote:
>> <graff.yang@xxxxxxxxx> 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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>>
>> Â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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/