| Message ID | 20200406023700.1367-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Add kvfree_sensitive() for freeing sensitive data objects | expand |
On Sun, 5 Apr 2020, 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 | 17 +++++++++++++++++ > security/keys/internal.h | 11 ----------- > security/keys/keyctl.c | 16 +++++----------- > 3 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 7dd5c4ccbf85..c26f279f1956 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -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 > + * @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))? > + memzero_explicit((void *)addr, len); > + kvfree(addr); > + } > +} > + > static inline int compound_mapcount(struct page *page) > { > VM_BUG_ON_PAGE(!PageCompound(page), page); > 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 > > >
David Rientjes <rientjes@google.com> wrote: > > +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. David
Waiman Long <longman@redhat.com> wrote:
> +static inline void kvfree_sensitive(const void *addr, size_t len)
Linus suggested making it non-inline.
David
On 4/6/20 12:20 AM, David Rientjes wrote: > On Sun, 5 Apr 2020, 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 | 17 +++++++++++++++++ >> security/keys/internal.h | 11 ----------- >> security/keys/keyctl.c | 16 +++++----------- >> 3 files changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 7dd5c4ccbf85..c26f279f1956 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -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 >> + * @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 if (likely(addr && len)) { Cheers, Longman
On Mon, Apr 06, 2020 at 10:36:07AM -0400, Waiman Long wrote: > 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 Why is all the kvalloc/kvfree crap in mm.h to begin with? slab.h is a more sensible place to put it. I had a patch to do that once, but I think it went stale before I posted it.
On 4/6/20 10:32 AM, David Howells wrote: > Waiman Long <longman@redhat.com> wrote: > >> +static inline void kvfree_sensitive(const void *addr, size_t len) > Linus suggested making it non-inline. Sorry, I misread his comment. Will send a v2 to fix that. Cheers, Longman
On Sun, 2020-04-05 at 22:37 -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. Why isn't this called kvzfree like the existing kzfree?
Joe Perches <joe@perches.com> wrote: > > 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". David
On Mon, 2020-04-06 at 17:00 +0100, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > > > > 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". 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.
Joe Perches <joe@perches.com> wrote: > 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. How much overhead would it be to always use kvfree_sensitive() and never have a kfree_sensitive()? David
On Mon, 2020-04-06 at 17:26 +0100, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > > > 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. > > How much overhead would it be to always use kvfree_sensitive() and never have > a kfree_sensitive()? I believe the is_vmalloc_addr not particularly expensive as it's just 2 tests. It might make sense to go back to static inline is_vmalloc_addr instead of using EXPORT_SYMBOL
On Mon, Apr 6, 2020 at 9:12 AM Joe Perches <joe@perches.com> wrote: > > 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. So using the same naming is wrong. They have one implementation detail that looks superficially similar ("zero the area"), but even that superficial similarity is actually completely false. They may both use "memset()", but in one case it is correct and makes sense, and in the other case it's actually a bug waiting to happen, and you really should use that "memzero_explicit()", which is a very very different operation from a normal memzero(). So even the implementation isn't really validly similar, but even if it had been, the _reason_ for doing so is completely different. They simply don't really pair up in any way. Linus
On Mon, 2020-04-06 at 09:41 -0700, Linus Torvalds wrote: > On Mon, Apr 6, 2020 at 9:12 AM Joe Perches <joe@perches.com> wrote: > > 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.
On Mon, 2020-04-06 at 17:26 +0100, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > > > 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. > > 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.
On Mon, Apr 6, 2020 at 9:44 AM Joe Perches <joe@perches.com> wrote: > > 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". Should you name all global variables with a 'z' in their name somewhere? They start out zeroed too - so pretty much according to your logic, they are exactly the same as 'kzalloc()'. Linus
On Mon, 2020-04-06 at 10:11 -0700, Linus Torvalds wrote: > On Mon, Apr 6, 2020 at 9:44 AM Joe Perches <joe@perches.com> wrote: > > Dubious assertion. Both end up with zeroed memory. > > You don't understand the function. Another dubious assertion. > 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. or memfill(0xdeadbeef). > 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". API function naming symmetry is good. You ignore or don't quote the kzfree/kfree_sensitive too. Yet I don't say _you_ don't understand something.
On Mon, Apr 06, 2020 at 10:10:20AM -0700, Joe Perches wrote: > On Mon, 2020-04-06 at 17:26 +0100, David Howells wrote: > > Joe Perches <joe@perches.com> wrote: > > > > > 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. > > > > 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. kfree() doesn't take GFP flags.
On Mon, Apr 06, 2020 at 10:20:24AM -0700, Joe Perches wrote: > > 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. It's good when there's actual symmetry between the two functions. kvalloc() memory should be freed with kvfree(). That makes sense. kzalloc() memory should not normally be freed with kzfree(). The symmetry hurts you, not helps you.
On Mon, Apr 6, 2020 at 10:12 AM Joe Perches <joe@perches.com> wrote: > > 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(). I'm not sure how controlled the allocations are, though. The allocations that get used for keys etc might come from outside the crypto layer. Linus
On Mon, Apr 6, 2020 at 10:22 AM Joe Perches <joe@perches.com> wrote: > > API function naming symmetry is good. BS. Naming should be symmetric if _use_ is symmetric. But if the use is completely different, then the naming should be completely different too. A symmetric naming is only helpful if it implies symmetries in use. Otherwise it's actively misleading. In "kzalloc()", the z is meaningful and an important part of what the caller wants. In "kzfree()", the z is actively detrimental, because maybe in the future we really _might_ want to use that "memfill(0xdeadbeef)" or something. The "zero" part of the interface isn't even _relevant_. See? There is no API symmetry. There is only a small and immaterial implementation detail. We don't put an "l" into the kfree/kmalloc names because they internally use a percpu list to manage the allocations, do we? That's a "symmetry" too. But it's an irrelevant implementation detail that makes no sense to the caller. Linus
On Mon, 2020-04-06 at 10:33 -0700, Linus Torvalds wrote: > On Mon, Apr 6, 2020 at 10:22 AM Joe Perches <joe@perches.com> wrote: > > API function naming symmetry is good. [] > See? There is no API symmetry. There is only a small and immaterial > implementation detail. The symmetry to an API is the existing 300+ kzfree calls. Renaming all the kzfree calls would be fine. Consolidating all the various types of kfree to just kfree might be even better. cheers, Joe
Linus Torvalds <torvalds@linux-foundation.org> wrote: > > 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. David
On Mon, Apr 6, 2020 at 10:51 AM David Howells <dhowells@redhat.com> wrote: > > The allocation might be by vmalloc rather than kmalloc. I'm not sure if that > makes things more difficult. It does add yet another place where we'd have to save the "this allocation is special", but it's not insurmountable. That said, I think the short-term and simple solution is to just teach people that sensitive free's are different, and at least have the key subsystem with sane naming. And yes, then eventually convert the existing crypto subsystem uses too for consistency. Linus
On 4/6/20 12:10 PM, Joe Perches wrote: > On Mon, 2020-04-06 at 17:00 +0100, David Howells wrote: >> Joe Perches <joe@perches.com> wrote: >> >>>> 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". > 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. Cheers, Longman
On Mon, Apr 6, 2020 at 10:59 AM Waiman Long <longman@redhat.com> wrote: > > 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. Somebody (maybe you) asked whether we could just use kvfree_sensitive() for everything, We probably could. The extra test is cheap - much cheaper than the memzero_explicit(). That said, _there_ I think that consistency with regular kfree/kvfree naming means that we might as well keep separate names, and keep the kmalloc->kfree_sensitive and kvmalloc->kvfree_sensitive pairing. Even if technically we could do with just the one function that works for both cases. Linus
On Mon, 2020-04-06 at 11:06 -0700, Linus Torvalds wrote: > On Mon, Apr 6, 2020 at 10:59 AM Waiman Long <longman@redhat.com> wrote: > > 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. Not really as comment and prototype and existing cocci scripts that contain kzfree are difficult to change. A sed is straightforward and works well. $ git grep -w --name-only kzfree | \ xargs sed -i 's/\bkzfree\b/kfree_sensitive/' For today's next that's: $ git diff --shortstat 116 files changed, 322 insertions(+), 322 deletions(-) After this change: The kernel-doc comment in slab_common.c should be edited from zeroed to something else. * kfree_sensitive - like kfree but zero memory * @p: object to free memory of * * The memory of the object @p points to is zeroed before freed. * If @p is %NULL, kfree_sensitive() does nothing. * * Note: this function zeroes the whole allocated buffer which can be a good * deal bigger than the requested buffer size passed to kmalloc(). So be * careful when using this function in performance sensitive code. */
On Mon, 6 Apr 2020, David Howells wrote: > David Rientjes <rientjes@google.com> wrote: > > > > +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. > Ah lol, yeah. Probably just better to do if (unlikely(ZERO_OR_NULL_PTR(addr))) return; but I agree that mm.h is likely not the right spot.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 7dd5c4ccbf85..c26f279f1956 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -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 + * @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) { + memzero_explicit((void *)addr, len); + kvfree(addr); + } +} + static inline int compound_mapcount(struct page *page) { VM_BUG_ON_PAGE(!PageCompound(page), page); 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 | 17 +++++++++++++++++ security/keys/internal.h | 11 ----------- security/keys/keyctl.c | 16 +++++----------- 3 files changed, 22 insertions(+), 22 deletions(-)