From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933232AbcLNUbh (ORCPT ); Wed, 14 Dec 2016 15:31:37 -0500 Received: from mail-io0-f175.google.com ([209.85.223.175]:34678 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753979AbcLNUbf (ORCPT ); Wed, 14 Dec 2016 15:31:35 -0500 MIME-Version: 1.0 In-Reply-To: <20161214185110.GD4939@kroah.com> References: <20161214185000.GA3930@kroah.com> <20161214185110.GD4939@kroah.com> From: Kees Cook Date: Wed, 14 Dec 2016 12:31:34 -0800 X-Google-Sender-Auth: pXNAaXdZKJPv3N6yFzO9SdC3YfE Message-ID: Subject: Re: [RFC 4/4] Introduce CONFIG_READONLY_USERMODEHELPER To: Greg KH Cc: "kernel-hardening@lists.openwall.com" , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 14, 2016 at 10:51 AM, Greg KH wrote: > If you can write to kernel memory, an "easy" way to get the kernel to > run any application is to change the pointer of one of the usermode > helper program names. To try to mitigate this, create a new config > option, CONFIG_READONLY_USERMODEHELPER. > > This option only allows "predefined" binaries to be called. A number of > drivers and subsystems allow for the name of the binary to be changed, > and this config option disables that capability, so be aware of that. > > Note: Still a proof-of-concept at this point in time, doesn't cover all > of the call_usermodehelper() calls just yet, including the "fun" of > coredumps, it's still a work in progress. > > Not-Signed-off-by: Greg Kroah-Hartman > --- > arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++---- > drivers/block/drbd/drbd_int.h | 6 +++++- > drivers/block/drbd/drbd_main.c | 5 +++++ > drivers/video/fbdev/uvesafb.c | 19 ++++++++++++++----- > fs/nfs/cache_lib.c | 12 ++++++++++-- > include/linux/reboot.h | 2 ++ > kernel/ksysfs.c | 6 +++++- > kernel/reboot.c | 3 +++ > kernel/sysctl.c | 4 ++++ > lib/kobject_uevent.c | 3 +++ > security/Kconfig | 17 +++++++++++++++++ > 11 files changed, 76 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c > index 00ef43233e03..92a2ef8ffe3e 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce.c > +++ b/arch/x86/kernel/cpu/mcheck/mce.c > @@ -2337,15 +2337,16 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr, > } > > static ssize_t > -show_trigger(struct device *s, struct device_attribute *attr, char *buf) > +trigger_show(struct device *s, struct device_attribute *attr, char *buf) > { > strcpy(buf, mce_helper); > strcat(buf, "\n"); > return strlen(mce_helper) + 1; The +1 is wrong, AFAICT. Also, is speed really needed here? return scnprintf(buf, PAGE_SIZE, "%s\n", mce_helper); is more readable... > -static ssize_t set_trigger(struct device *s, struct device_attribute *attr, > - const char *buf, size_t siz) > +#ifndef CONFIG_READONLY_USERMODEHELPER > +static ssize_t trigger_store(struct device *s, struct device_attribute *attr, > + const char *buf, size_t siz) > { > char *p; > > @@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr, > > return strlen(mce_helper) + !!p; > } > +static DEVICE_ATTR_RW(trigger); > +#else > +static DEVICE_ATTR_RO(trigger); > +#endif > > static ssize_t set_ignore_ce(struct device *s, > struct device_attribute *attr, > @@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s, > return ret; > } > > -static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger); > static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant); > static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout); > static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce); > diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h > index a139a34f1f1e..e21ab2bcc482 100644 > --- a/drivers/block/drbd/drbd_int.h > +++ b/drivers/block/drbd/drbd_int.h > @@ -75,7 +75,11 @@ extern int fault_rate; > extern int fault_devs; > #endif > > -extern char drbd_usermode_helper[]; > +extern > +#ifdef CONFIG_READONLY_USERMODEHELPER > + const > +#endif > + char drbd_usermode_helper[]; This #ifdef; const; #endif is repeated a few times. Perhaps better to create a separate macro: #ifdef CONFIG_READONLY_USERMODEHELPER # define __ro_umh const #else # define __ro_umh /**/ #endif ... extern __ro_umh char drbd_usermode_helper[]; -Kees -- Kees Cook Nexus Security