linux-kernel - Re: [GIT PULL] keys: Fix key->sem vs mmap

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]

Date:   Sat, 4 Apr 2020 13:00:08 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Howells <dhowells@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Waiman Long <longman@...hat.com>, keyrings@...r.kernel.org,
        LSM List <linux-security-module@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

On Mon, Mar 30, 2020 at 5:16 AM David Howells <dhowells@...hat.com> wrote:
>
>  security/keys/internal.h                  |  12 ++++

This isn't so much about this pull (which I have taken), as about the
fact that this code re-inforces bad behavior we already in the slub
layer, and now extends it further to kvfree.

Doing this:

   __kvzfree(const void *addr, size_t len)
  ..
                memset((void *)addr, 0, len);
                kvfree(addr);

is wrong to begin with. It's wrong because if the compiler ever knows
that kvfree is a freeing function (with something like
__attribute__((free)) - I don't think gcc is smart enough today), the
compiler might throw the memset away.

Yeah, so far we've only seen that for automatic stack clearing, but
there are very much compilers that know that alloc/free are special
(both for warning about use-after-free issues, and for "improving"
code generation by blindly removing dead writes).

We have a function for clearing sensitive information: it's called
"memclear_explicit()", and it's about forced (explicit) clearing even
if the data might look dead afterwards.

The other problem with that function is the name: "__kvzfree()" is not
a useful name for this function. We use the "__" format for internal
low-level helpers, and it generally means that it does *less* than the
full function. This does more, not less, and "__" is not following any
sane naming model.

So the name should probably be something like "kvfree_sensitive()" or
similar. Or maybe it could go even further, and talk about _why_ it's
sensitive, and call it "kvfree_cleartext()" or something like that.

Because the clearing is really not what even matters. It might choose
other patterns to overwrite things with, but it might do other things
too, like putting special barriers for data leakage (or flags to tell
return-to-user-mode to do so).

And yes, kzfree() isn't a good name either, and had that same
memset(), but at least it doesn't do the dual-underscore mistake.

Including some kzfree()/crypto people explicitly - I hope we can get
away from this incorrect and actively wrong pattern of thinking that
"sensitive data should be memset(), and then we should add a random
'z' in the name somewhere to 'document' that".

               Linus