linux-kernel - Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171104015805.o6kkoydzn7kfhiav@salmiak>
Date:   Sat, 4 Nov 2017 01:58:05 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     Kees Cook <keescook@...omium.org>,
        Laura Abbott <labbott@...hat.com>,
        kernel-hardening@...ts.openwall.com,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>
Subject: Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

On Sat, Nov 04, 2017 at 12:24:30AM +0000, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

FWIW, on the arm64 version I'd used BUG() since my initial focus was fuzzing.

We can make this safe for hardening by returning -EFAULT instead.

> > > +config PARANOID_UACCESS
> > > +       bool "Use paranoid uaccess primitives"
> > > +       depends on ARCH_HAS_PARANOID_UACCESS
> > > +       help
> > > +         Forces access_ok() checks in __get_user(), __put_user(), and other
> > > +         low-level uaccess primitives which usually do not have checks. This
> > > +         can limit the effect of missing access_ok() checks in higher-level
> > > +         primitives, with a runtime performance overhead in some cases and a
> > > +         small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

Certainly that's where we want to get to.

However, in the mean time, I'd argue that there's little downside to providing
the option to check the nominally unchecked primitives, provided this is
optional.

In investigating this for arm/arm64, I found at least one other bug. I would
not be surprised to find that I had missed others, nor would I be surprised to
find that new bugs get introduced in future.

Thanks,
Mark.