* [PATCH] Honor mmap_min_addr with the actual minimum @ 2016-04-06 19:07 Hector Marco-Gisbert 2016-04-06 22:40 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Hector Marco-Gisbert @ 2016-04-06 19:07 UTC (permalink / raw) To: linux-kernel Cc: James Morris, Serge E. Hallyn, linux-security-module, kees Cook, Ismael Ripoll, Hector Marco-Gisbert 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. 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@upv.es> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-04-06 19:07 [PATCH] Honor mmap_min_addr with the actual minimum Hector Marco-Gisbert @ 2016-04-06 22:40 ` Kees Cook 2016-04-19 18:55 ` Hector Marco-Gisbert 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2016-04-06 22:40 UTC (permalink / raw) To: Hector Marco-Gisbert Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll, Eric Paris On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi@upv.es> 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 > > 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@upv.es> > Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> > --- > 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 > -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-04-06 22:40 ` Kees Cook @ 2016-04-19 18:55 ` Hector Marco-Gisbert 2016-04-20 22:12 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Hector Marco-Gisbert @ 2016-04-19 18:55 UTC (permalink / raw) To: Kees Cook Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll, Eric Paris 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 ? Thanks, Hector. > On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi@upv.es> 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 > >> >> 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@upv.es> >> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> >> --- >> 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-04-19 18:55 ` Hector Marco-Gisbert @ 2016-04-20 22:12 ` Kees Cook 2016-05-11 12:54 ` Hector Marco-Gisbert 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2016-04-20 22:12 UTC (permalink / raw) To: Hector Marco-Gisbert Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll, Eric Paris On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@upv.es> wrote: >> On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi@upv.es> 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 ? 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@upv.es> >>> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> >>> --- >>> 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) -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-04-20 22:12 ` Kees Cook @ 2016-05-11 12:54 ` Hector Marco-Gisbert 2016-05-11 13:50 ` Eric Paris 0 siblings, 1 reply; 7+ messages in thread From: Hector Marco-Gisbert @ 2016-05-11 12:54 UTC (permalink / raw) To: Kees Cook Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll, Eric Paris El 21/04/16 a las 00:12, Kees Cook escribió: > On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@upv.es> wrote: >>> On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi@upv.es> 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@upv.es> >>>> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> >>>> --- >>>> 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-05-11 12:54 ` Hector Marco-Gisbert @ 2016-05-11 13:50 ` Eric Paris 2016-05-12 17:56 ` Hector Marco-Gisbert 0 siblings, 1 reply; 7+ messages in thread From: Eric Paris @ 2016-05-11 13:50 UTC (permalink / raw) To: Hector Marco-Gisbert, Kees Cook Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll On Wed, 2016-05-11 at 14:54 +0200, Hector Marco-Gisbert wrote: > > El 21/04/16 a las 00:12, Kees Cook escribió: > > On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@up > > v.es> wrote: > > > > On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi > > > > @upv.es> 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 ? This breaks scripts which are currently setting mmap_min_addr (like wine on ubuntu I think?). Seems like a non-starter. You're trying to represent multiple values in a single value. It just isn't possible. You could expose lsm_mmap_min_addr RO in another sysctl (not sure of other places we expose kernel configs like that, but you could). I wouldn't say the meaning of mmap_min_addr changed, we just grew a new (underdocumented) lsm_mmap_min_addr. mmap_min_addr continued to be controlled by and controlling exactly the same thing. dac_mmap_min_addr is controlled by capabilities. lsm_mmap_min_addr is controlled by your LSM. You can expose those 2 values. But it would be us to each process to know how to use them. A process might be able to avoid the dac check but not the mac check (aka a root process) or a process might be able to avoid the mac check but not the dac check (wine). No single value can represent this. The best you could do is expose the lsm/mac value, but I'm not sure I see the value. All you are doing is telling exploit authors exactly how high they have to put their nasty bits... > > 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@upv.es> > > > > > Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> > > > > > --- > > > > > 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) > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Honor mmap_min_addr with the actual minimum 2016-05-11 13:50 ` Eric Paris @ 2016-05-12 17:56 ` Hector Marco-Gisbert 0 siblings, 0 replies; 7+ messages in thread From: Hector Marco-Gisbert @ 2016-05-12 17:56 UTC (permalink / raw) To: Eric Paris, Kees Cook Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module, Ismael Ripoll Thanks for the clarification. Below some comments. > On Wed, 2016-05-11 at 14:54 +0200, Hector Marco-Gisbert wrote: >> >> El 21/04/16 a las 00:12, Kees Cook escribió: >>> On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@up >>> v.es> wrote: >>>>> On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi >>>>> @upv.es> 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 ? > > This breaks scripts which are currently setting mmap_min_addr (like > wine on ubuntu I think?). Seems like a non-starter. > > You're trying to represent multiple values in a single value. It just > isn't possible. You could expose lsm_mmap_min_addr RO in another sysctl > (not sure of other places we expose kernel configs like that, but you > could). > > I wouldn't say the meaning of mmap_min_addr changed, we just grew a new > (underdocumented) lsm_mmap_min_addr. mmap_min_addr continued to be > controlled by and controlling exactly the same thing. > > dac_mmap_min_addr is controlled by capabilities. > lsm_mmap_min_addr is controlled by your LSM. > > You can expose those 2 values. But it would be us to each process to > know how to use them. A process might be able to avoid the dac check > but not the mac check (aka a root process) or a process might be able > to avoid the mac check but not the dac check (wine). > > No single value can represent this. The best you could do is expose the > lsm/mac value, but I'm not sure I see the value. All you are doing is > telling exploit authors exactly how high they have to put their nasty > bits... > Well, you can set the permissions of this file to 0600 or even use SELINUX to prevent exploit authors to read this value :) In any case if dac value is lower than lsm then any userspace application can obtain the lsm value by justing calling mmap(). All these issues comes out because we are working in a new ASLR (ASLRNG) and it would be useful to known "the worst case" (the maximum of SELINUX and CAP or any other) at user level just for testing purposes. But we agree that is not a big issue and maybe does not worth to export that value. Best, Hector. >> >> 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@upv.es> >>>>>> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es> >>>>>> --- >>>>>> 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-12 17:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-06 19:07 [PATCH] Honor mmap_min_addr with the actual minimum Hector Marco-Gisbert 2016-04-06 22:40 ` Kees Cook 2016-04-19 18:55 ` Hector Marco-Gisbert 2016-04-20 22:12 ` Kees Cook 2016-05-11 12:54 ` Hector Marco-Gisbert 2016-05-11 13:50 ` Eric Paris 2016-05-12 17:56 ` Hector Marco-Gisbert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.