Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Hector Marco-Gisbert]

El 21/04/16 a las 00:12, Kees Cook escribiÃ:
> On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@xxxxxx> wrote:
>>> On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi@xxxxxx> wrote:
>>>> The minimum address that a process is allowed to mmap when LSM is
>>>> enabled is 0x10000 (65536). This value is tunable and exported via
>>>> /proc/sys/vm/mmap_min_addr but it is not honored with the actual
>>>> minimum value.
>>>
>>> I think this is working as intended already, based on the commit log
>>> for 788084aba2ab7348257597496befcbccabdc98a3
>>>
>>> See cap_mmap_addr (which uses dac_mmap_min_addr) vs SELinux's hook
>>> (which uses CONFIG_LSM_MMAP_MIN_ADDR), and everything else (that uses
>>> mmap_min_addr).
>>>
>>> Without CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr always == mmap_min_addr.
>>>
>>> With CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr can be less than
>>> mmap_min_addr, but mmap_min_addr will always be at least
>>> CONFIG_LSM_MMAP_MIN_ADDR.
>>>
>>> Eric may be able to shed more light on this...
>>>
>>> -Kees
>>
>> Ok, I see your point, but it seems that minimum address that a process is
>> allowed to map is mmap_min_addr and not dac_mmap_min_addr.
>> This is because mmap_min_addr can be seen as the max(dac_mmap_min_addr,
>> CONFIG_LSM_MMAP_MIN_ADDR) which is correct (the minimum allowed address) but
>> /proc/sys/vm/mmap_min_addr contains dac_mmap_min_addr which is not the minimum.
>>
>> For example, if we set the CONFIG_LSM_MMAP_MIN_ADDR to 65536 and
>> /proc/sys/vm/mmap_min_addr to 4096, then assuming that selinux_mmap_addr() has
>> no permissions (it returns !=0), the minimum allowed address is 65536 not 4096.
>> The mmap check is done in the security_mmap_addr(addr) function in mm/mmap.c
>> file. It seems that we are exporting the dac_mmap_min_addr instead of the actual
>> minimum.
>>
>> Is this behavior intended ? I'm missing something here ?

Yes, the sysctl is reporting the dac value.

I think the meaning of the exported mmap_min_addr value was changed in the
commit you pointed. A new variable was added (dac_mmap_min_addr) and it was
replaced in the sysctl of "mmap_min_addr" but the exported name
(/proc/sys/vm/mmap_min_addr) was not changed:

.procname = "mmap_min_addr",
- .data = &mmap_min_addr,
+ .data = &dac_mmap_min_addr,

This can be confusing since the returned value is not the expected one (the
minimum value according to sysctl/vm.txt) but the dac_mmap_min_addr. So, I think
that If we need to export the dac value then we can do it but it would be
desirable not to change the meaning of this exported value.

Maybe by renaming /proc/sys/vm/mmap_min_addr to /proc/sys/vm/dac_mmap_min_addr
and adding a read-only /proc/sys/vm/mmap_min_addr ?

If ok, I could send a patch.

In any case, I think we should update the doc (sysctl/vm.txt).

All these issue came to light because we are working on a new ASLR for userspace
and for testing it would be easier if we know where the VMA starts (this can be
changed at runtime and it affects to the available entropy).


Best,
Hector.

> 
> I think it is -- the minimum is correct, it's just that the sysctl may
> be reporting the dac value. Eric, are you able to chime in on this?
> 
> -Kees
> 
>>
>> Thanks,
>> Hector.
>>
>>>
>>>>
>>>> It can be easily checked in a system typing:
>>>>
>>>> $ cat /proc/sys/vm/mmap_min_addr
>>>> 4096    # <= Incorrect, it should be 65536
>>>>
>>>> $ echo 1024 > /proc/sys/vm/mmap_min_addr
>>>> $ cat /proc/sys/vm/mmap_min_addr
>>>> 1024    # <= Incorrect, it should be 65536
>>>>
>>>> After applying the patch:
>>>>
>>>> $ cat /proc/sys/vm/mmap_min_addr
>>>> 65536    # <= It is correct
>>>>
>>>> $ echo 1024 > /proc/sys/vm/mmap_min_addr
>>>> $ cat /proc/sys/vm/mmap_min_addr
>>>> 65536    # <= It is correct
>>>>
>>>>
>>>>
>>>> Signed-off-by: Hector Marco-Gisbert <hecmargi@xxxxxx>
>>>> Acked-by: Ismael Ripoll Ripoll <iripoll@xxxxxx>
>>>> ---
>>>>  security/min_addr.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/security/min_addr.c b/security/min_addr.c
>>>> index f728728..96d1811 100644
>>>> --- a/security/min_addr.c
>>>> +++ b/security/min_addr.c
>>>> @@ -15,10 +15,12 @@ unsigned long dac_mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
>>>>  static void update_mmap_min_addr(void)
>>>>  {
>>>>  #ifdef CONFIG_LSM_MMAP_MIN_ADDR
>>>> -       if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR)
>>>> +       if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR) {
>>>>                 mmap_min_addr = dac_mmap_min_addr;
>>>> -       else
>>>> +       } else {
>>>>                 mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
>>>> +               dac_mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
>>>> +       }
>>>>  #else
>>>>         mmap_min_addr = dac_mmap_min_addr;
>>>>  #endif
>>>> --
>>>> 1.9.1
>>>>
>>>
>>>
>>>
>>
>> --
>> Dr. Hector Marco-Gisbert @ http://hmarco.org/
>> Cyber Security Researcher @ http://cybersecurity.upv.es
>> Universitat PolitÃcnica de ValÃncia (Spain)
> 
> 
> 

-- 
Dr. Hector Marco-Gisbert @ http://hmarco.org/
Cyber Security Researcher @ http://cybersecurity.upv.es
Universitat PolitÃcnica de ValÃncia (Spain)