[GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[David Howells]

Hi Linus,

Here's a couple of patches that fix a circular dependency between holding
key->sem and mm->mmap_sem when reading data from a key.  One potential
issue is that a filesystem looking to use a key inside, say, ->readpages()
could deadlock if the key being read is the key that's required and the
buffer the key is being read into is on a page that needs to be fetched.

The case actually detected is a bit more involved - with a filesystem
calling request_key() and locking the target keyring for write - which
could be being read.

[Note: kbuild spotted a compiler(?) warning that I've not seen before,
 complaining "The scope of the variable 'oldxdr' can be reduced.
 [variableScope]".  It's unhappy that a variable that's declared at the top
 of the function hasn't been moved into an interior for-loop.  Is this
 something we're now requiring?  Anyway, I'd prefer to fix that with a
 follow up patch through the net tree rather than go for a 9th iteration on
 these patches.]

Thanks,
David
---
The following changes since commit 1b649e0bcae71c118c1333e02249a7510ba7f70a:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2020-03-25 13:58:05 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20200329

for you to fetch changes up to 4f0882491a148059a52480e753b7f07fc550e188:

  KEYS: Avoid false positive ENOMEM error on key read (2020-03-29 12:40:41 +0100)

----------------------------------------------------------------
Keyrings fixes

----------------------------------------------------------------
Waiman Long (2):
      KEYS: Don't write out to userspace while holding key semaphore
      KEYS: Avoid false positive ENOMEM error on key read

 include/keys/big_key-type.h               |   2 +-
 include/keys/user-type.h                  |   3 +-
 include/linux/key-type.h                  |   2 +-
 net/dns_resolver/dns_key.c                |   2 +-
 net/rxrpc/key.c                           |  27 +++-----
 security/keys/big_key.c                   |  11 ++--
 security/keys/encrypted-keys/encrypted.c  |   7 +-
 security/keys/internal.h                  |  12 ++++
 security/keys/keyctl.c                    | 103 +++++++++++++++++++++++++-----
 security/keys/keyring.c                   |   6 +-
 security/keys/request_key_auth.c          |   7 +-
 security/keys/trusted-keys/trusted_tpm1.c |  14 +---
 security/keys/user_defined.c              |   5 +-
 13 files changed, 126 insertions(+), 75 deletions(-)

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[Linus Torvalds]

On Mon, Mar 30, 2020 at 5:16 AM David Howells <dhowells@redhat.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

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[Waiman Long]

On 4/4/20 4:00 PM, Linus Torvalds wrote:
> On Mon, Mar 30, 2020 at 5:16 AM David Howells <dhowells@redhat.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
>
Thanks for the suggestion, I will post a patch to rename the function to
kvzfree_explicit() and use memzero_explicit() for clearing memory.

Cheers,
Longman

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[David Howells]

Waiman Long <longman@redhat.com> wrote:

> > 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
> >
> Thanks for the suggestion, I will post a patch to rename the function to
> kvzfree_explicit() and use memzero_explicit() for clearing memory.

Should this be moved into core code, rather than being squirrelled away in
security/keys/?

David

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[Linus Torvalds]

On Sun, Apr 5, 2020 at 2:04 AM David Howells <dhowells@redhat.com> wrote:
>
> Should this be moved into core code, rather than being squirrelled away in
> security/keys/?

Yes. I do think that that __kvzfree() function makes sense in general
(the same way that kzfree does).

I just happen to despise the name, and think that the implementation
isn't great.

It also probably makes no sense to make it an inline function. It's
not like that function is done for performance reasons, and it might
only get worse if we then end up making it cause barriers or something
for CPU data leakage issues or whatever.

           Linus

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[Waiman Long]

On 4/5/20 1:31 PM, Linus Torvalds wrote:
> On Sun, Apr 5, 2020 at 2:04 AM David Howells <dhowells@redhat.com> wrote:
>> Should this be moved into core code, rather than being squirrelled away in
>> security/keys/?
> Yes. I do think that that __kvzfree() function makes sense in general
> (the same way that kzfree does).
>
> I just happen to despise the name, and think that the implementation
> isn't great.
>
> It also probably makes no sense to make it an inline function. It's
> not like that function is done for performance reasons, and it might
> only get worse if we then end up making it cause barriers or something
> for CPU data leakage issues or whatever.
>
>            Linus
>
I have just posted a patch that modify the API as suggested. Please let
me know if further change is needed.

Cheers,
Longman

Re: [GIT PULL] keys: Fix key->sem vs mmap_sem issue when reading key

[pr-tracker-bot]

The pull request you sent on Mon, 30 Mar 2020 13:16:38 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20200329

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4c205c84e249e0a91dcfabe461d77667ec9b2d05

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker