[PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Waiman Long]

For kvmalloc'ed data object that contains sensitive information like
cryptographic key, we need to make sure that the buffer is always
cleared before freeing it. Using memset() alone for buffer clearing may
not provide certainty as the compiler may compile it away. To be sure,
the special memzero_explicit() has to be used.

This patch introduces a new kvfree_sensitive() for freeing those
sensitive data objects allocated by kvmalloc(). The relevnat places
where kvfree_sensitive() can be used are modified to use it.

Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/mm.h       |  1 +
 mm/util.c                | 18 ++++++++++++++++++
 security/keys/internal.h | 11 -----------
 security/keys/keyctl.c   | 16 +++++-----------
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7dd5c4ccbf85..9b3130b20f42 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -757,6 +757,7 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 }
 
 extern void kvfree(const void *addr);
+extern void kvfree_sensitive(const void *addr, size_t len);
 
 static inline int compound_mapcount(struct page *page)
 {
diff --git a/mm/util.c b/mm/util.c
index 988d11e6c17c..01e210766f3d 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -604,6 +604,24 @@ void kvfree(const void *addr)
 }
 EXPORT_SYMBOL(kvfree);
 
+/**
+ * kvfree_sensitive - free a data object containing sensitive information
+ * @addr - address of the data object to be freed
+ * @len  - length of the data object
+ *
+ * Use the special memzero_explicit() function to clear the content of a
+ * kvmalloc'ed object containing sensitive data to make sure that the
+ * compiler won't optimize out the data clearing.
+ */
+void kvfree_sensitive(const void *addr, size_t len)
+{
+	if (likely(!ZERO_OR_NULL_PTR(addr))) {
+		memzero_explicit((void *)addr, len);
+		kvfree(addr);
+	}
+}
+EXPORT_SYMBOL(kvfree_sensitive);
+
 static inline void *__page_rmapping(struct page *page)
 {
 	unsigned long mapping;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 6d0ca48ae9a5..153d35c20d3d 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -350,15 +350,4 @@ static inline void key_check(const struct key *key)
 #define key_check(key) do {} while(0)
 
 #endif
-
-/*
- * Helper function to clear and free a kvmalloc'ed memory object.
- */
-static inline void __kvzfree(const void *addr, size_t len)
-{
-	if (addr) {
-		memset((void *)addr, 0, len);
-		kvfree(addr);
-	}
-}
 #endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 5e01192e222a..edde63a63007 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -142,10 +142,7 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 
 	key_ref_put(keyring_ref);
  error3:
-	if (payload) {
-		memzero_explicit(payload, plen);
-		kvfree(payload);
-	}
+	kvfree_sensitive(payload, plen);
  error2:
 	kfree(description);
  error:
@@ -360,7 +357,7 @@ long keyctl_update_key(key_serial_t id,
 
 	key_ref_put(key_ref);
 error2:
-	__kvzfree(payload, plen);
+	kvfree_sensitive(payload, plen);
 error:
 	return ret;
 }
@@ -914,7 +911,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 */
 		if (ret > key_data_len) {
 			if (unlikely(key_data))
-				__kvzfree(key_data, key_data_len);
+				kvfree_sensitive(key_data, key_data_len);
 			key_data_len = ret;
 			continue;	/* Allocate buffer */
 		}
@@ -923,7 +920,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 			ret = -EFAULT;
 		break;
 	}
-	__kvzfree(key_data, key_data_len);
+	kvfree_sensitive(key_data, key_data_len);
 
 key_put_out:
 	key_put(key);
@@ -1225,10 +1222,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
 		keyctl_change_reqkey_auth(NULL);
 
 error2:
-	if (payload) {
-		memzero_explicit(payload, plen);
-		kvfree(payload);
-	}
+	kvfree_sensitive(payload, plen);
 error:
 	return ret;
 }
-- 
2.18.1

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Joe Perches]

On Mon, 2020-04-06 at 14:58 -0400, Waiman Long wrote:
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.

[] 
>  extern void kvfree(const void *addr);
> +extern void kvfree_sensitive(const void *addr, size_t len);

Question: why should this be const?

2.1.44 changed kfree(void *) to kfree(const void *) but
I didn't find a particular reason why.

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Matthew Wilcox]

On Mon, Apr 06, 2020 at 02:58:27PM -0400, Waiman Long wrote:
> +/**
> + * kvfree_sensitive - free a data object containing sensitive information
> + * @addr - address of the data object to be freed
> + * @len  - length of the data object

Did you try building this with W=1?  I believe this is incorrect kerneldoc.
It should be @addr: and @len:

Also, it reads better in the htmldocs if you capitalise the first letter
of each sentence and finish with a full stop.

> @@ -914,7 +911,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  		 */
>  		if (ret > key_data_len) {
>  			if (unlikely(key_data))
> -				__kvzfree(key_data, key_data_len);
> +				kvfree_sensitive(key_data, key_data_len);

I'd drop the test of key_data here.

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Waiman Long]

On 4/6/20 3:38 PM, Joe Perches wrote:
> On Mon, 2020-04-06 at 14:58 -0400, Waiman Long wrote:
>> For kvmalloc'ed data object that contains sensitive information like
>> cryptographic key, we need to make sure that the buffer is always
>> cleared before freeing it. Using memset() alone for buffer clearing may
>> not provide certainty as the compiler may compile it away. To be sure,
>> the special memzero_explicit() has to be used.
> [] 
>>  extern void kvfree(const void *addr);
>> +extern void kvfree_sensitive(const void *addr, size_t len);
> Question: why should this be const?
>
> 2.1.44 changed kfree(void *) to kfree(const void *) but
> I didn't find a particular reason why.

I am just following the function prototype used by kvfree(). Even
kzfree(const void *) use const. I can remove "const" if others agree.

Cheers,
Longman

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Joe Perches]

On Mon, 2020-04-06 at 22:16 -0400, Waiman Long wrote:
> On 4/6/20 3:38 PM, Joe Perches wrote:
> > On Mon, 2020-04-06 at 14:58 -0400, Waiman Long wrote:
> > > For kvmalloc'ed data object that contains sensitive information like
> > > cryptographic key, we need to make sure that the buffer is always
> > > cleared before freeing it. Using memset() alone for buffer clearing may
> > > not provide certainty as the compiler may compile it away. To be sure,
> > > the special memzero_explicit() has to be used.
> > [] 
> > >  extern void kvfree(const void *addr);
> > > +extern void kvfree_sensitive(const void *addr, size_t len);
> > Question: why should this be const?
> > 
> > 2.1.44 changed kfree(void *) to kfree(const void *) but
> > I didn't find a particular reason why.
> 
> I am just following the function prototype used by kvfree(). Even
> kzfree(const void *) use const. I can remove "const" if others agree.

No worries.  Nevermind me...

Lots of warnings if allocated pointers are const, so const is necessary
in the definition and declaration.

struct foo {
	...
};

struct bar {
	const struct foo *baz;
	...
};

some_func(void)
{
	bar.baz = kvalloc(...);
}

kvfree can't free bar.baz if it's defined with void * without warning,
so it must be const void *.

Apologies for the noise.

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Waiman Long]

On 4/6/20 4:00 PM, Matthew Wilcox wrote:
> On Mon, Apr 06, 2020 at 02:58:27PM -0400, Waiman Long wrote:
>> +/**
>> + * kvfree_sensitive - free a data object containing sensitive information
>> + * @addr - address of the data object to be freed
>> + * @len  - length of the data object
> Did you try building this with W=1?  I believe this is incorrect kerneldoc.
> It should be @addr: and @len:
>
> Also, it reads better in the htmldocs if you capitalise the first letter
> of each sentence and finish with a full stop.
>
You are right. I use the wrong delimiter here. I just send out a v3
patch to fix that. Thanks for noticing it.


>> @@ -914,7 +911,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>>  		 */
>>  		if (ret > key_data_len) {
>>  			if (unlikely(key_data))
>> -				__kvzfree(key_data, key_data_len);
>> +				kvfree_sensitive(key_data, key_data_len);
> I'd drop the test of key_data here.
>
I would like to keep the unlikely tag here to emphaize the fact that
this path should not be taken. I have comments up a few lines to talk
about it, though it didn't show up in the diff.

Cheers,
Longman

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Linus Torvalds]

On Mon, Apr 6, 2020 at 12:40 PM Joe Perches <joe@perches.com> wrote:
>
> 2.1.44 changed kfree(void *) to kfree(const void *) but
> I didn't find a particular reason why.

Because "free()" should always have been const (and volatile, for that
matter, but the kernel doesn't care since we eschew volatile data
structures).

It's a bug in the C library standard.

Think of it this way: free() doesn't really change the data, it kills
the lifetime of it. You can't access it afterwards - you can neither
read it nor write it validly. That is a completely different - and
independent - operation from writing to it.

And  more importantly, it's perfectly fine to have a const data
structure (or a volatile one) that you free. The allocation may have
done something like this:

   struct mystruct {
       const struct dictionary *dictionary;
       ...
   };

and it was allocated and initialized before it was assigned to that
"dictionary" pointer. That's _good_ code.

So it wasn't const before the allocation, but it turned const
afterwards, and freeing it doesn't change that, it just kills the
lifetime entirely.

So "free()" should take a const pointer without complaining, and saying

   free(mystruct->dictionary);
   free(mystruct);

is a sensible an correct thing to do. Warning about - or requiring
that dictionary pointer to be cast to be freed - is fundamentally
wrong.

We're not bound by the fact that the C standard library got their
rules wrong, so we can fix it in the kernel.

             Linus

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Linus Torvalds]

On Tue, Apr 7, 2020 at 1:16 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Think of it this way: free() doesn't really change the data, it kills
> the lifetime of it. You can't access it afterwards - you can neither
> read it nor write it validly. That is a completely different - and
> independent - operation from writing to it.

Side note: I'd really love to be able to describe that operation, but
there's sadly no such extension.

So the _real_ prototype for 'free()'-like operations should be something like

    void free(const volatile killed void *ptr);

where that "killed" also tells the compiler that the pointer lifetime
is dead, so that using it afterwards is invalid. So that the compiler
could warn us about some of the most trivial use-after-free cases.

Because we've had even those trivially stupid ones

Yes, obviously various analysis systems do exactly that kind of
analysis (and usually go much further), but then it's external things
like coverity etc.

The point being that the lifetime of an object is independent from
being able to write to an object, and the "const" in the "free()" is
not "I promise to not write to it", but "I can accept a constant
pointer".

We've had a number of places in the kernel where we do that kind of
"lifetime" marking explicitly by assigning a NULL (or invalid value)
to the pointer when we free it.

I have this dim memory of us even (long long long ago) trying to use a
#define kfree() ... to do that, but it turns out to be basically
impossible to get the proper "use once" semantics, so it doesn't work
if the argument to kfree() has side effects.

               Linus

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So the _real_ prototype for 'free()'-like operations should be something like
> 
>     void free(const volatile killed void *ptr);
> 
> where that "killed" also tells the compiler that the pointer lifetime
> is dead, so that using it afterwards is invalid. So that the compiler
> could warn us about some of the most trivial use-after-free cases.

It might be worth asking the compiler folks to give us an __attribute__ for
that - even if they don't do anything with it immediately.  So we might have
something like:

	void free(const volatile void *ptr) __attribute__((free(1)));

There are some for allocation functions, some of which we use, though I'm not
sure we do so as consistently as we should (should inline functions like
kcalloc() have them, for example?).

David

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Linus Torvalds]

On Tue, Apr 7, 2020 at 2:14 PM David Howells <dhowells@redhat.com> wrote:
>
> It might be worth asking the compiler folks to give us an __attribute__ for
> that - even if they don't do anything with it immediately.  So we might have
> something like:
>
>         void free(const volatile void *ptr) __attribute__((free(1)));

Yeah, that sounds sane.

> There are some for allocation functions, some of which we use, though I'm not
> sure we do so as consistently as we should (should inline functions like
> kcalloc() have them, for example?).

I think that gcc supports a "malloc" attribute, but it's only used for
alias analysis optimizations, afaik (ie it says that the pointer the
function returns cannot alias anything else).

So we do have that "__malloc" thing, but I'm not sure how much it
actually matters.

And adding it to inline functions shouldn't be _wrong_, but it
shouldn't matter either, since I think the alias analysis would work
regardless.

I wonder how much of a code generation difference it makes. I suspect
not a lot, but maybe I'd be surprsied.

But yes, having the free attribute would be consistent (even if the
syntax for it might be as you suggest, kind of like the __printf()
attribute works). Even if it wasn't initially used for anything it
wouldn't hurt, and maybe some day it would improve warnings (and allow
the compiler to do the dead store elimination that started this whole
long set of threads in the first place..)

            Linus

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Matthew Wilcox]

On Tue, Apr 07, 2020 at 10:14:11PM +0100, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > So the _real_ prototype for 'free()'-like operations should be something like
> > 
> >     void free(const volatile killed void *ptr);
> > 
> > where that "killed" also tells the compiler that the pointer lifetime
> > is dead, so that using it afterwards is invalid. So that the compiler
> > could warn us about some of the most trivial use-after-free cases.
> 
> It might be worth asking the compiler folks to give us an __attribute__ for
> that - even if they don't do anything with it immediately.  So we might have
> something like:
> 
> 	void free(const volatile void *ptr) __attribute__((free(1)));
> 
> There are some for allocation functions, some of which we use, though I'm not
> sure we do so as consistently as we should (should inline functions like
> kcalloc() have them, for example?).

GCC recognises free() as being a __builtin.  I don't know if there's
an __attribute__ for it.

gcc/builtins.def:DEF_LIB_BUILTIN        (BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)

It looks like the only two things this really does is warn you if you
try to free a pointer that gcc can prove isn't in the heap, and elide
the call if gcc can prove it's definitely NULL.  Which are both things
that a compiler should do, but aren't all that valuable.

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > It might be worth asking the compiler folks to give us an __attribute__ for
> > that - even if they don't do anything with it immediately.  So we might have
> > something like:
> >
> >         void free(const volatile void *ptr) __attribute__((free(1)));
> 
> Yeah, that sounds sane.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527

> Even if it wasn't initially used for anything it wouldn't hurt, and maybe
> some day it would improve warnings (and allow the compiler to do the dead
> store elimination that started this whole long set of threads in the first
> place..)

With regard to this, I've got back "not sure what Linus was talking about WRT
DSE, if he's got examples he could pass along, they'd be appreciated"

David

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Linus Torvalds]

On Tue, Apr 7, 2020 at 3:54 PM David Howells <dhowells@redhat.com> wrote:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527
>
> With regard to this, I've got back "not sure what Linus was talking about WRT
> DSE, if he's got examples he could pass along, they'd be appreciated"

I'll do that. We have real examples in the kernel, although they
probably aren't all that _important_.

I don't see that comment on the bugzilla, but I'll put the stupid
example in there too.

One such example would be kernel/async.c: async_run_entry_fn(), where we have

        /* 2) remove self from the pending queues */
        spin_lock_irqsave(&async_lock, flags);
        list_del_init(&entry->domain_list);
        list_del_init(&entry->global_list);

        /* 3) free the entry */
        kfree(entry);
        atomic_dec(&entry_count);

and while it's good form to do "list_del_init()" on those fields in
entry, the fact that we then do "kfree(entry)" right afterwards means
that the stores that re-initialize the entry list are dead.

So _if_ we had some way to tell the compiler that "hey, kfree(ptr)
kills the lifetime of that object", the compiler could eliminate the
dead stores.

I think that dead store elimination is perhaps less important than if
the compiler could warn about us stupidly using the dead storage
afterwards, but I mention it as a "it can actually matter for code
generation" example too.

Now, the above is a particularly stupid example, because if we cared,
we could just turn the "list_del_init()" into a plain "list_del()",
and simply not do the unnecessary re-initialization of the list entry
after removing it.

But I picked a stupid example because it's easy to understand.

Less stupidly, we sometimes have "cleanup" functions that get rid of
things, and are called before you free the underlying storage.

And there, the cleanup function might be used in general, and not only
just before freeing. So the re-initialization could make sense in that
context, but might again be just dead stores for the actual final
freeing case.

Is this a big deal? No it's not. But it's not really any different
from the dead store elimination that gcc already does for local
variables on stack.

          Linus

Re: [PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Waiman Long]

On 4/7/20 4:01 PM, Waiman Long wrote:
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.
>
> This patch introduces a new kvfree_sensitive() for freeing those
> sensitive data objects allocated by kvmalloc(). The relevnat places
> where kvfree_sensitive() can be used are modified to use it.
>
> Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  include/linux/mm.h       |  1 +
>  mm/util.c                | 18 ++++++++++++++++++
>  security/keys/internal.h | 11 -----------
>  security/keys/keyctl.c   | 16 +++++-----------
>  4 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7dd5c4ccbf85..9b3130b20f42 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -757,6 +757,7 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
>  }
>  
>  extern void kvfree(const void *addr);
> +extern void kvfree_sensitive(const void *addr, size_t len);
>  
>  static inline int compound_mapcount(struct page *page)
>  {
> diff --git a/mm/util.c b/mm/util.c
> index 988d11e6c17c..01e210766f3d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -604,6 +604,24 @@ void kvfree(const void *addr)
>  }
>  EXPORT_SYMBOL(kvfree);
>  
> +/**
> + * kvfree_sensitive - free a data object containing sensitive information
> + * @addr - address of the data object to be freed
> + * @len  - length of the data object
> + *
> + * Use the special memzero_explicit() function to clear the content of a
> + * kvmalloc'ed object containing sensitive data to make sure that the
> + * compiler won't optimize out the data clearing.
> + */
> +void kvfree_sensitive(const void *addr, size_t len)
> +{
> +	if (likely(!ZERO_OR_NULL_PTR(addr))) {
> +		memzero_explicit((void *)addr, len);
> +		kvfree(addr);
> +	}
> +}
> +EXPORT_SYMBOL(kvfree_sensitive);
> +
>  static inline void *__page_rmapping(struct page *page)
>  {
>  	unsigned long mapping;
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 6d0ca48ae9a5..153d35c20d3d 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -350,15 +350,4 @@ static inline void key_check(const struct key *key)
>  #define key_check(key) do {} while(0)
>  
>  #endif
> -
> -/*
> - * Helper function to clear and free a kvmalloc'ed memory object.
> - */
> -static inline void __kvzfree(const void *addr, size_t len)
> -{
> -	if (addr) {
> -		memset((void *)addr, 0, len);
> -		kvfree(addr);
> -	}
> -}
>  #endif /* _INTERNAL_H */
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 5e01192e222a..edde63a63007 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -142,10 +142,7 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
>  
>  	key_ref_put(keyring_ref);
>   error3:
> -	if (payload) {
> -		memzero_explicit(payload, plen);
> -		kvfree(payload);
> -	}
> +	kvfree_sensitive(payload, plen);
>   error2:
>  	kfree(description);
>   error:
> @@ -360,7 +357,7 @@ long keyctl_update_key(key_serial_t id,
>  
>  	key_ref_put(key_ref);
>  error2:
> -	__kvzfree(payload, plen);
> +	kvfree_sensitive(payload, plen);
>  error:
>  	return ret;
>  }
> @@ -914,7 +911,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  		 */
>  		if (ret > key_data_len) {
>  			if (unlikely(key_data))
> -				__kvzfree(key_data, key_data_len);
> +				kvfree_sensitive(key_data, key_data_len);
>  			key_data_len = ret;
>  			continue;	/* Allocate buffer */
>  		}
> @@ -923,7 +920,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  			ret = -EFAULT;
>  		break;
>  	}
> -	__kvzfree(key_data, key_data_len);
> +	kvfree_sensitive(key_data, key_data_len);
>  
>  key_put_out:
>  	key_put(key);
> @@ -1225,10 +1222,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
>  		keyctl_change_reqkey_auth(NULL);
>  
>  error2:
> -	if (payload) {
> -		memzero_explicit(payload, plen);
> -		kvfree(payload);
> -	}
> +	kvfree_sensitive(payload, plen);
>  error:
>  	return ret;
>  }

Sorry, wrong one. Please ignore it.

-Longman

[PATCH v2] mm: Add kvfree_sensitive() for freeing sensitive data objects

[Waiman Long]

For kvmalloc'ed data object that contains sensitive information like
cryptographic key, we need to make sure that the buffer is always
cleared before freeing it. Using memset() alone for buffer clearing may
not provide certainty as the compiler may compile it away. To be sure,
the special memzero_explicit() has to be used.

This patch introduces a new kvfree_sensitive() for freeing those
sensitive data objects allocated by kvmalloc(). The relevnat places
where kvfree_sensitive() can be used are modified to use it.

Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/mm.h       |  1 +
 mm/util.c                | 18 ++++++++++++++++++
 security/keys/internal.h | 11 -----------
 security/keys/keyctl.c   | 16 +++++-----------
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7dd5c4ccbf85..9b3130b20f42 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -757,6 +757,7 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 }
 
 extern void kvfree(const void *addr);
+extern void kvfree_sensitive(const void *addr, size_t len);
 
 static inline int compound_mapcount(struct page *page)
 {
diff --git a/mm/util.c b/mm/util.c
index 988d11e6c17c..01e210766f3d 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -604,6 +604,24 @@ void kvfree(const void *addr)
 }
 EXPORT_SYMBOL(kvfree);
 
+/**
+ * kvfree_sensitive - free a data object containing sensitive information
+ * @addr - address of the data object to be freed
+ * @len  - length of the data object
+ *
+ * Use the special memzero_explicit() function to clear the content of a
+ * kvmalloc'ed object containing sensitive data to make sure that the
+ * compiler won't optimize out the data clearing.
+ */
+void kvfree_sensitive(const void *addr, size_t len)
+{
+	if (likely(!ZERO_OR_NULL_PTR(addr))) {
+		memzero_explicit((void *)addr, len);
+		kvfree(addr);
+	}
+}
+EXPORT_SYMBOL(kvfree_sensitive);
+
 static inline void *__page_rmapping(struct page *page)
 {
 	unsigned long mapping;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 6d0ca48ae9a5..153d35c20d3d 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -350,15 +350,4 @@ static inline void key_check(const struct key *key)
 #define key_check(key) do {} while(0)
 
 #endif
-
-/*
- * Helper function to clear and free a kvmalloc'ed memory object.
- */
-static inline void __kvzfree(const void *addr, size_t len)
-{
-	if (addr) {
-		memset((void *)addr, 0, len);
-		kvfree(addr);
-	}
-}
 #endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 5e01192e222a..edde63a63007 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -142,10 +142,7 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 
 	key_ref_put(keyring_ref);
  error3:
-	if (payload) {
-		memzero_explicit(payload, plen);
-		kvfree(payload);
-	}
+	kvfree_sensitive(payload, plen);
  error2:
 	kfree(description);
  error:
@@ -360,7 +357,7 @@ long keyctl_update_key(key_serial_t id,
 
 	key_ref_put(key_ref);
 error2:
-	__kvzfree(payload, plen);
+	kvfree_sensitive(payload, plen);
 error:
 	return ret;
 }
@@ -914,7 +911,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 */
 		if (ret > key_data_len) {
 			if (unlikely(key_data))
-				__kvzfree(key_data, key_data_len);
+				kvfree_sensitive(key_data, key_data_len);
 			key_data_len = ret;
 			continue;	/* Allocate buffer */
 		}
@@ -923,7 +920,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 			ret = -EFAULT;
 		break;
 	}
-	__kvzfree(key_data, key_data_len);
+	kvfree_sensitive(key_data, key_data_len);
 
 key_put_out:
 	key_put(key);
@@ -1225,10 +1222,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
 		keyctl_change_reqkey_auth(NULL);
 
 error2:
-	if (payload) {
-		memzero_explicit(payload, plen);
-		kvfree(payload);
-	}
+	kvfree_sensitive(payload, plen);
 error:
 	return ret;
 }
-- 
2.18.1