mm: Add kvfree_sensitive() for freeing sensitive data objects

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 <[email protected]>

Signed-off-by Waiman Long <[email protected]>

---
 include/linux/mm.h       | 17 +++++++++++++++++
 security/keys/internal.h | 11 -----------
 security/keys/keyctl.c   | 16 +++++-----------
 3 files changed, 22 insertions(+), 22 deletions(-)

include/linux/mm.h +2 -0

index 7dd5c4ccbf85..c26f279f1956 100644
@@ -758,6 +758,23 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 
 extern void kvfree(const void *addr);
 
+/**
+ * kvfree_sensitive - free a data object containing sensitive information

include/linux/mm.h +5 -0

+		memzero_explicit((void *)addr, len);
+		kvfree(addr);
+	}
+}
+
 static inline int compound_mapcount(struct page *page)
 {
 	VM_BUG_ON_PAGE(!PageCompound(page), page);

security/keys/internal.h +0 -11

index 6d0ca48ae9a5..153d35c20d3d 100644
@@ -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 */

security/keys/keyctl.c +5 -11

index 5e01192e222a..edde63a63007 100644
@@ -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;
 }

Signature · 1 line

2.18.1

include/linux/mm.h static inline void *kvcalloc(size_t n, size_t size, gfp_t flags) · line 772

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline void kvfree_sensitive(const void *addr, size_t len)
+{
+	if (addr) {

Shouldn't this be if (unlikely(ZERO_OR_NULL_PTR(addr))?

Quoted 5 lines · David Rientjes

include/linux/mm.h static inline void *kvcalloc(size_t n, size_t size, gfp_t flags) · line 772

Quoted 3 lines

include/linux/mm.h +3 -0

+static inline void kvfree_sensitive(const void *addr, size_t len)
+{
+	if (addr) {

Shouldn't this be if (unlikely(ZERO_OR_NULL_PTR(addr))?

You've reversed the logic - it needs a '!' there.

Quoted 27 lines · 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 <[email protected]>

Signed-off-by Waiman Long <[email protected]>

---
 include/linux/mm.h       | 17 +++++++++++++++++
 security/keys/internal.h | 11 -----------
 security/keys/keyctl.c   | 16 +++++-----------
 3 files changed, 22 insertions(+), 22 deletions(-)

include/linux/mm.h +0 -0

index 7dd5c4ccbf85..c26f279f1956 100644
@@ -758,6 +758,23 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 
 extern void kvfree(const void *addr);
 

include/linux/mm.h static inline void *kvcalloc(size_t n, size_t size, gfp_t flags) · line 772

Quoted 12 lines

include/linux/mm.h +12 -0

+/**
+ * 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.
+ */
+static inline void kvfree_sensitive(const void *addr, size_t len)
+{
+	if (addr) {

Shouldn't this be if (unlikely(ZERO_OR_NULL_PTR(addr))?

ZERO_OR_NULL_PTR is defined in slab.h. Using it may cause some header file dependency problem. To guard against the possibility of 0-length allocation request, how about just

+static inline void kvfree_sensitive(const void *addr, size_t len)

Quoted 1 line · Waiman Long

+static inline void kvfree_sensitive(const void *addr, size_t len)

Linus suggested making it non-inline.

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.

Quoted 3 lines

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.

Why isn't this called kvzfree like the existing kzfree?

Quoted 5 lines · Joe Perches

Quoted 3 lines

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.

Why isn't this called kvzfree like the existing kzfree?

To quote Linus:

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".

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

And if _sensitive is actually used, it'd be
good to do a s/kzfree/kfree_sensitive/ one day
sooner than later.

Quoted 4 lines · Joe Perches

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

Quoted 3 lines

And if _sensitive is actually used, it'd be
good to do a s/kzfree/kfree_sensitive/ one day
sooner than later.

How much overhead would it be to always use kvfree_sensitive() and never have a kfree_sensitive()?

Quoted 4 lines · Joe Perches

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

Quoted 3 lines

And if _sensitive is actually used, it'd be
good to do a s/kzfree/kfree_sensitive/ one day
sooner than later.

How much overhead would it be to always use kvfree_sensitive() and never have a kfree_sensitive()?

Quoted 10 lines · David Howells

Quoted 7 lines · Joe Perches

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

And if _sensitive is actually used, it'd be
good to do a s/kzfree/kfree_sensitive/ one day
sooner than later.

Quoted 2 lines

How much overhead would it be to always use kvfree_sensitive() and never have a kfree_sensitive()?

Another possibility:

Add yet another alloc flag like __GFP_SENSITIVE
and have kfree operate on that and not have a
kfree_sensitive at all.

Add yet another alloc flag like __GFP_SENSITIVE
and have kfree operate on that and not have a
kfree_sensitive at all.

Quoted 3 lines

Add yet another alloc flag like __GFP_SENSITIVE
and have kfree operate on that and not have a
kfree_sensitive at all.

That sounds potentially sensible. Maybe even a SLAB_SENSITIVE to mark a whole slab cache sensitive for kmem_cache_create().

The allocation might be by vmalloc rather than kmalloc. I'm not sure if that makes things more difficult.

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

Quoted 3 lines · Joe Perches

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

Yes, we have a pattern of 'z' for zero.

But the _operation_ isn't symmetric.

"kzalloc()" has absolutely _nothing_ to do with "kzfree()". They are not some kind of "opposite symmetric operation". They are totally different. They have absolutely nothing in common.

Dubious assertion. Both end up with zeroed memory.

Quoted 1 line · Joe Perches

Dubious assertion. Both end up with zeroed memory.

You don't understand the function.

You ignored the part where the zeroed memory isn't even the _point_.

Yes, for kzalloc() it is. There the zero is inherent and important. People very much depend on it, and it's the whole point of that function. The 'z' is not silent.

But for kzfree() it really really isn't. There the zeroing is never going to be seen by anybody wjho does the right thing, and is not important at all - it's purely a "let's make sure old contents don't leak".

The "zero" part is completely immaterial, it could just as well have been a "memset(0xaa)" instead.

And you didn't seem to understand that kzfree() shouldn't use memset() in the first place, so it's not even using the same operation.

You really don't seem to get the whole "kzfree() has absolutely _nothing_ to do with kzalloc() apart from a dubious implementation details".

Quoted 3 lines

You really don't seem to get the whole "kzfree() has absolutely _nothing_ to do with kzalloc() apart from a dubious implementation details".

API function naming symmetry is good.

API function naming symmetry is good.

Quoted 1 line · Joe Perches

API function naming symmetry is good.

See? There is no API symmetry. There is only a small and immaterial implementation detail.

Quoted 27 lines · David Howells

Quoted 4 lines · Joe Perches

Quoted 3 lines

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.

Why isn't this called kvzfree like the existing kzfree?

To quote Linus:

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).

Quoted 7 lines

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".

Thanks.

While I agree with Linus about the __ prefix,
the z is pretty common and symmetric to all
the <foo>zalloc uses.

And if _sensitive is actually used, it'd be
good to do a s/kzfree/kfree_sensitive/ one day
sooner than later.

I have actually been thinking about that. I saw a couple of cases in the crypto code where a memzero_explicit() is followed by kfree(). Those can be replaced by kfree_sensitive.

Quoted 3 lines · Waiman Long

I have actually been thinking about that. I saw a couple of cases in the crypto code where a memzero_explicit() is followed by kfree(). Those can be replaced by kfree_sensitive.

Ack.

Doing that (and renaming kvzfree) should be a fairly straightforward coccinelle patch.