From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932716AbcEKNup (ORCPT ); Wed, 11 May 2016 09:50:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58872 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932452AbcEKNuo (ORCPT ); Wed, 11 May 2016 09:50:44 -0400 Message-ID: <1462974641.12064.54.camel@redhat.com> Subject: Re: [PATCH] Honor mmap_min_addr with the actual minimum From: Eric Paris To: Hector Marco-Gisbert , Kees Cook Cc: LKML , James Morris , "Serge E. Hallyn" , linux-security-module , Ismael Ripoll Date: Wed, 11 May 2016 09:50:41 -0400 In-Reply-To: <57332B95.7020902@upv.es> References: <1459969648-4386-1-git-send-email-hecmargi@upv.es> <57167F16.6090507@upv.es> <57332B95.7020902@upv.es> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 11 May 2016 13:50:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > v.es> wrote: > > > > On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert > > > @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 > > > > > Acked-by: Ismael Ripoll Ripoll > > > > > --- > > > > >  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) > > > > > > >