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.

sensitive data objects allocated by kvmalloc(). The relevnat places

security/keys/keyctl.c long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) · line 914

Quoted 3 lines

security/keys/keyctl.c +1 -1

 			if (unlikely(key_data))
-				__kvzfree(key_data, key_data_len);
+				kvfree_sensitive(key_data, key_data_len);

I think the if-statement is redundant.

Quoted 5 lines · David Howells

security/keys/keyctl.c long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) · line 914

Quoted 3 lines

security/keys/keyctl.c +1 -1

 			if (unlikely(key_data))
-				__kvzfree(key_data, key_data_len);
+				kvfree_sensitive(key_data, key_data_len);

I think the if-statement is redundant.

Ah - I see that you explicitly wanted to keep it.

There's a good chance it'll get janitored at some point.

Quoted 7 lines · On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <[email protected]> wrote:

Quoted 4 lines · David Howells

Review

Quoted 3 lines

if (unlikely(key_data))

(quoted diff) +1 -1

-				__kvzfree(key_data, key_data_len);
+				kvfree_sensitive(key_data, key_data_len);

I think the if-statement is redundant.

Ah - I see that you explicitly wanted to keep it.

Why's that?

Quoted 1 line

There's a good chance it'll get janitored at some point.

Indeed. Perhaps add a few little comments to explain the reasoning and to keep the janitorial fingers away?

Quoted 1 line · Waiman Long

sensitive data objects allocated by kvmalloc(). The relevnat places

"relevant".

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

[]

include/linux/mm.h

Quoted 1 line

include/linux/mm.h +0 -0

[]

Quoted 3 lines

include/linux/mm.h +0 -0

@@ -757,6 +757,7 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 }
 

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

Quoted 2 lines

include/linux/mm.h +1 -0

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

Why should size_t len be required?

Why not do what kzfree does and memset
the entire allocation? (area->size)

If the memory is really virtually mapped, the only way to find out the size of the object is to use find_vm_area() which can be relatively high cost and no simple helper function is available.

Quoted 4 lines · Waiman Long

If the memory is really virtually mapped, the only way to find out the size of the object is to use find_vm_area() which can be relatively high cost and no simple helper function is available.

We _could_ just push it down to a "vfree_sensitive()", and do it inside the vfree logic. That ends up obviously figuring out the size of the area eventually.

But since the vmalloc data structures fundamentally aren't irq-safe, vfree() actually has magical things like "if called in an interrupt, we'll delay it to work context".

Seems like there is only one place where we can "sleep". I mean when we call vfree(). That is free_vmap_area_noflush() -> try_purge_vmap_area_lazy(). Basically try_purge_vmap_area_lazy() can call the schedule() what is not allowed for IRQs. Instead of inlining the try_purge_vmap_area_lazy() into current context we can schedule_work(). And i think it makes sense from many point of views.

Quoted 4 lines · Joe Perches

Quoted 1 line · Waiman Long

+extern void kvfree_sensitive(const void *addr, size_t len);

Why should size_t len be required?

Quoted 2 lines

Why not do what kzfree does and memset
the entire allocation? (area->size)

If the memory is really virtually mapped, the only way to find out the size of the object is to use find_vm_area() which can be relatively high cost and no simple helper function is available. On the other hand, the length is readily available in the callers. So passing the length directly to the kvfree_sensitive is simpler.

Quoted 5 lines · Waiman Long

Quoted 4 lines · Joe Perches

Quoted 1 line · Waiman Long

+extern void kvfree_sensitive(const void *addr, size_t len);

Why should size_t len be required?

Quoted 8 lines

Quoted 2 lines

Why not do what kzfree does and memset
the entire allocation? (area->size)

If the memory is really virtually mapped, the only way to find out the size of the object is to use find_vm_area() which can be relatively high cost and no simple helper function is available. On the other hand, the length is readily available in the callers. So passing the length directly to the kvfree_sensitive is simpler.

Also it lets us zero only the first N bytes of the allocation. That might be good for performance, if only the first N bytes of an M byte allocation are actually sensitive. I don't know if we have any such cases, but they could exist.

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]>

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]>

Quoted 6 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.

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

Fixes 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")

Suggested-by Linus Torvalds <[email protected]>

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

Looks good, feel free to add:

Reviewed-by Eric Biggers <[email protected]>

(I don't really buy the argument that the compiler could compile away memset() before kvfree(). But I agree with using memzero_explicit() anyway to make the intent explicit.)

I don't see this patch in linux-next yet. Who is planning to take this patch? Presumably David through the keyrings tree, or Andrew through mm?

- Eric

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       |  1 +
 mm/util.c                | 18 ++++++++++++++++++
 security/keys/internal.h | 11 -----------
 security/keys/keyctl.c   | 16 +++++-----------
 4 files changed, 24 insertions(+), 22 deletions(-)

[v3: Fix kerneldoc errors]

include/linux/mm.h +1 -0

index 7dd5c4ccbf85..9b3130b20f42 100644
@@ -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)
 {

mm/util.c +8 -0

index 988d11e6c17c..dc1c877d5481 100644
@@ -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.

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)

{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}

EXPORT_SYMBOL(kvfree_sensitive);

Quoted 11 lines · Balbir Singh

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)

{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}

Quoted 1 line

EXPORT_SYMBOL(kvfree_sensitive);

If it's _that_ sensitive then the caller should have disabled preemption. Because preemption could otherwise have occurred immediately before kvfree_sensitive() was called.

Quoted 11 lines · Balbir Singh

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)

{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}

Quoted 1 line

EXPORT_SYMBOL(kvfree_sensitive);

If it's _that_ sensitive then the caller should have disabled preemption. Because preemption could otherwise have occurred immediately before kvfree_sensitive() was called.

Quoted 14 lines · Matthew Wilcox

Quoted 12 lines · Balbir Singh

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)

{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}

EXPORT_SYMBOL(kvfree_sensitive);

Quoted 4 lines

If it's _that_ sensitive then the caller should have disabled preemption. Because preemption could otherwise have occurred immediately before kvfree_sensitive() was called.

May be, but the callers of the API have to be explictly aware of the contract. I don't disagree with you on what you've said, but I was referring to the intent of freeing sensitive data vs the turn around time for doing so.

Quoted 13 lines · Matthew Wilcox

Quoted 12 lines · Balbir Singh

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)

{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}

EXPORT_SYMBOL(kvfree_sensitive);

Quoted 4 lines

If it's _that_ sensitive then the caller should have disabled preemption. Because preemption could otherwise have occurred immediately before kvfree_sensitive() was called.

May be, but the callers of the API have to be explictly aware of the contract. I don't disagree with you on what you've said, but I was referring to the intent of freeing sensitive data vs the turn around time for doing so.