| Message ID | 20200407200318.11711-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm: Add kvfree_sensitive() for freeing sensitive data objects | expand |
On Tue, Apr 7, 2020 at 1:03 PM Waiman Long <longman@redhat.com> 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. Ack. Since this isn't exactly high-priority, I'm assuming it will go through the usual channels (ie Andrew). Linus
Waiman Long <longman@redhat.com> wrote: > sensitive data objects allocated by kvmalloc(). The relevnat places "relevant". > if (unlikely(key_data)) > - __kvzfree(key_data, key_data_len); > + kvfree_sensitive(key_data, key_data_len); I think the if-statement is redundant. David
David Howells <dhowells@redhat.com> wrote: > > 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. David
On 4/7/20 4:19 PM, David Howells wrote: > Waiman Long <longman@redhat.com> wrote: > >> sensitive data objects allocated by kvmalloc(). The relevnat places > "relevant". Oh, sorry about the typo. Maybe Andrew can fix it. Cheers, Longman
On Tue, 2020-04-07 at 16:03 -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. > > 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. [] > diff --git 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); Why should size_t len be required? Why not do what kzfree does and memset the entire allocation? (area->size)
On 4/7/20 4:31 PM, Joe Perches wrote: > On Tue, 2020-04-07 at 16:03 -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. >> >> 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. > [] >> diff --git 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); > 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. On the other hand, the length is readily available in the callers. So passing the length directly to the kvfree_sensitive is simpler. Cheers, Longman
On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <longman@redhat.com> wrote: > > 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". So that "eventually" can be quite a bit later, and it would delay the overwriting of the sensitive data if we did that. So this patch does end up simpler, but for vfree data it is actually technically the better approach too (since overwriting the sensitive data asap is what you want). Linus
On Tue, Apr 07, 2020 at 02:01:01PM -0700, Linus Torvalds wrote: > On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <longman@redhat.com> wrote: > > > > 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". > Just some thoughts. Sorry for jumping in. 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. Also, we can end up in zeroed non-existance vmap area if we do not find_vmap_area(). Thanks! -- Vlad Rezki
On Tue, Apr 7, 2020 at 2:25 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > 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. I don't think that's the only case. Or rather, that may be the only case of _sleeping_, but we also aren't irq-safe wrt locking. And I'm not just talking about the vmap_purge_lock mutex, but all the spinlocks etc we have. That said, I haven't looked at that code in _ages_. Maybe those things would be trivial to just turn into irq-safe ones and there are no real latency issues anywhere. Linus
On Tue, Apr 07, 2020 at 04:45:45PM -0400, Waiman Long wrote: > On 4/7/20 4:31 PM, Joe Perches wrote: > > On Tue, 2020-04-07 at 16:03 -0400, Waiman Long wrote: > >> +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. 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.
On Tue, 2020-04-07 at 15:12 -0700, Matthew Wilcox wrote: > On Tue, Apr 07, 2020 at 04:45:45PM -0400, Waiman Long wrote: > > On 4/7/20 4:31 PM, Joe Perches wrote: > > > On Tue, 2020-04-07 at 16:03 -0400, Waiman Long wrote: > > > > +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. 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. I would really doubt it as the allocation of sensitive data should generally be separate. Also, a similar argument could apply to kzfree/kfree_sensitive.
On Tue, Apr 07, 2020 at 04:03:18PM -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. > > 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> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> David, you want to pick this one? /Jarkko
On Tue, Apr 07, 2020 at 04:03:18PM -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. > > 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> Looks good, feel free to add: Reviewed-by: Eric Biggers <ebiggers@google.com> (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
On 5/1/20 7:22 PM, Eric Biggers wrote: > On Tue, Apr 07, 2020 at 04:03:18PM -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. >> >> 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> > Looks good, feel free to add: > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > (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 > Andrew, would you mind taking this patch into the mm-tree? Thanks, Longman
On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <dhowells@redhat.com> wrote: > David Howells <dhowells@redhat.com> wrote: > > > > 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. Why's that? > 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?
On 5/5/20 4:35 PM, Andrew Morton wrote: > On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <dhowells@redhat.com> wrote: > >> David Howells <dhowells@redhat.com> wrote: >> >>>> 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. > Why's that? There is a comment above it: /* * The key may change (unlikely) in between 2 consecutive * __keyctl_read_key() calls. In this case, we reallocate * a larger buffer and redo the key read when * key_data_len < ret <= buflen. */ if (ret > key_data_len) { if (unlikely(key_data)) __kvzfree(key_data, key_data_len); key_data will be defined only if the unlikely case that the key increase in length between the 2 consecutive __keyctl_read_key() call and we have to enlarge the buffer and read the key again. I want to keep the unlikely() macro to emphasize the fact that this condition should not happen. >> 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? > I can reword the comment to make it more explicit and send a v4 if you think the current comment is not clear enough. Cheers, Longman
On 8/4/20 6:03 am, 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(-) > > [v3: Fix kerneldoc errors] > > 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..dc1c877d5481 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); > + 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); Balbir Singh.
On Thu, May 14, 2020 at 09:00:40PM +1000, Balbir Singh wrote: > 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); If it's _that_ sensitive then the caller should have disabled preemption. Because preemption could otherwise have occurred immediately before kvfree_sensitive() was called.
On Thu, 2020-05-14 at 05:00 -0700, Matthew Wilcox wrote: > On Thu, May 14, 2020 at 09:00:40PM +1000, Balbir Singh wrote: > > 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); > > If it's _that_ sensitive then the caller should have disabled preemption. > Because preemption could otherwise have occurred immediately before > kvfree_sensitive() was called. static inline ?
On 14/5/20 10:00 pm, Matthew Wilcox wrote: > On Thu, May 14, 2020 at 09:00:40PM +1000, Balbir Singh wrote: >> 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); > > 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. Balbir Singh.
On Sun, May 17, 2020 at 10:27:39AM +1000, Balbir Singh wrote: > On 14/5/20 10:00 pm, Matthew Wilcox wrote: > > On Thu, May 14, 2020 at 09:00:40PM +1000, Balbir Singh wrote: > >> 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); > > > > 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. It's the caller's information. They should be aware of their own requirements. If they do something like: p = kmalloc(); preempt_disable(); construct(p); use(p); preempt_enable(); kvfree_sensitive(p); there's really nothing we can do to help them inside kvfree_sensitive(). Actually, can you come up with a scenario where disabling preemption inside kvfree_sensitive() will help with anything?
On 5/16/20 8:27 PM, Balbir Singh wrote: > > On 14/5/20 10:00 pm, Matthew Wilcox wrote: >> On Thu, May 14, 2020 at 09:00:40PM +1000, Balbir Singh wrote: >>> 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); >> 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. We can't disable preemption like that. The vfree() call may potentially sleep. It could be a mess to keep track of the preemption state to make that works. The purpose of this API is to make sure that a newly allocated memory block won't contain secret left behind from another task. There is no guarantee on how long the freeing process will take. Cheers, Longman
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..dc1c877d5481 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; }
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(-) [v3: Fix kerneldoc errors]