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(-)
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;
}
--
2.18.1
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 <[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(-)
>
> 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 <[email protected]> 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 <[email protected]> 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 <[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(-)
>>
>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 10:11 -0700, Linus Torvalds wrote:
> On Mon, Apr 6, 2020 at 9:44 AM Joe Perches <[email protected]> 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 <[email protected]> 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 9:44 AM Joe Perches <[email protected]> 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 17:26 +0100, David Howells wrote:
> Joe Perches <[email protected]> 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 10:12 AM Joe Perches <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 4/6/20 12:10 PM, Joe Perches wrote:
> On Mon, 2020-04-06 at 17:00 +0100, David Howells wrote:
>> Joe Perches <[email protected]> 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:51 AM David Howells <[email protected]> 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 Mon, Apr 6, 2020 at 10:59 AM Waiman Long <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.