2020-04-07 20:05:51

by Waiman Long

[permalink] [raw]
Subject: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

For kvmalloc'ed data object that contains sensitive information like
cryptographic key, we need to make sure that the buffer is always
cleared before freeing it. Using memset() alone for buffer clearing may
not provide certainty as the compiler may compile it away. To be sure,
the special memzero_explicit() has to be used.

This patch introduces a new kvfree_sensitive() for freeing those
sensitive data objects allocated by kvmalloc(). The relevnat places
where kvfree_sensitive() can be used are modified to use it.

Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Waiman Long <[email protected]>
---
include/linux/mm.h | 1 +
mm/util.c | 18 ++++++++++++++++++
security/keys/internal.h | 11 -----------
security/keys/keyctl.c | 16 +++++-----------
4 files changed, 24 insertions(+), 22 deletions(-)

[v3: Fix kerneldoc errors]

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;
}
--
2.18.1


2020-04-07 20:10:21

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On Tue, Apr 7, 2020 at 1:03 PM Waiman Long <[email protected]> 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

2020-04-07 20:21:04

by David Howells

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

Waiman Long <[email protected]> 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

2020-04-07 20:24:40

by David Howells

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

David Howells <[email protected]> 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

2020-04-07 20:25:24

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On 4/7/20 4:19 PM, David Howells wrote:
> Waiman Long <[email protected]> wrote:
>
>> sensitive data objects allocated by kvmalloc(). The relevnat places
> "relevant".

Oh, sorry about the typo. Maybe Andrew can fix it.

Cheers,
Longman

2020-04-07 20:34:15

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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)


2020-04-07 20:48:08

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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

2020-04-07 21:02:32

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <[email protected]> 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

2020-04-07 21:25:57

by Uladzislau Rezki

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On Tue, Apr 07, 2020 at 02:01:01PM -0700, Linus Torvalds wrote:
> On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <[email protected]> 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

2020-04-07 21:32:34

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On Tue, Apr 7, 2020 at 2:25 PM Uladzislau Rezki <[email protected]> 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

2020-04-07 22:14:33

by Matthew Wilcox (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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.

2020-04-08 00:38:37

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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.


2020-04-08 14:50:16

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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 <[email protected]>
> Signed-off-by: Waiman Long <[email protected]>

Acked-by: Jarkko Sakkinen <[email protected]>

David, you want to pick this one?

/Jarkko

2020-05-01 23:24:22

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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 <[email protected]>
> Signed-off-by: Waiman Long <[email protected]>

Looks good, feel free to add:

Reviewed-by: Eric Biggers <[email protected]>

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

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

- Eric

2020-05-04 03:26:52

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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 <[email protected]>
>> Signed-off-by: Waiman Long <[email protected]>
> Looks good, feel free to add:
>
> Reviewed-by: Eric Biggers <[email protected]>
>
> (I don't really buy the argument that the compiler could compile away memset()
> before kvfree(). But I agree with using memzero_explicit() anyway to make the
> intent explicit.)
>
> I don't see this patch in linux-next yet. Who is planning to take this patch?
> Presumably David through the keyrings tree, or Andrew through mm?
>
> - Eric
>
Andrew, would you mind taking this patch into the mm-tree?

Thanks,
Longman

2020-05-05 20:37:34

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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

> David Howells <[email protected]> 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?

2020-05-06 01:32:22

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

On 5/5/20 4:35 PM, Andrew Morton wrote:
> On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <[email protected]> wrote:
>
>> David Howells <[email protected]> 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

2020-05-14 11:02:49

by Balbir Singh

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects


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 <[email protected]>
> Signed-off-by: Waiman Long <[email protected]>
> ---
> include/linux/mm.h | 1 +
> mm/util.c | 18 ++++++++++++++++++
> security/keys/internal.h | 11 -----------
> security/keys/keyctl.c | 16 +++++-----------
> 4 files changed, 24 insertions(+), 22 deletions(-)
>
> [v3: Fix kerneldoc errors]
>
> 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.

2020-05-14 12:04:58

by Matthew Wilcox (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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.

2020-05-14 12:13:00

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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 ?

2020-05-17 00:30:14

by Balbir Singh

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects



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.

2020-05-17 00:50:09

by Matthew Wilcox (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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?

2020-05-18 02:43:56

by Waiman Long

[permalink] [raw]
Subject: Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects

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