| Message ID | 20200406185827.22249-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: Add kvfree_sensitive() for freeing sensitive data objects | expand |
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.
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.
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
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.
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
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
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
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
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
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.
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
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
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; }
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(-)