[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 14 Dec 2016 12:57:39 -0800
From: Greg KH <gregkh@...uxfoundation.org>
To: Kees Cook <keescook@...omium.org>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 4/4] Introduce CONFIG_READONLY_USERMODEHELPER
On Wed, Dec 14, 2016 at 12:31:34PM -0800, Kees Cook wrote:
> On Wed, Dec 14, 2016 at 10:51 AM, Greg KH <gregkh@...uxfoundation.org> 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 <gregkh@...uxfoundation.org>
> > ---
> > 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?
No, this is sysfs files, no speed at all :)
> return scnprintf(buf, PAGE_SIZE, "%s\n", mce_helper);
>
> is more readable...
true, that's nicer, I was trying not to change things that I didn't have
to.
> > -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[];
Ah, much cleaner, thanks, I'll go do something like that. After fixing
up the static const crap I got wrong...
greg k-h
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
