[v3] mm: Add kvfree_sensitive() for freeing sensitive data objects
diff mbox series

Message ID 20200407200318.11711-1-longman@redhat.com
State In Next
Commit 35c4051c4a0dd7d9f233ff0adb5a01145b474831
Headers show
Series
  • [v3] mm: Add kvfree_sensitive() for freeing sensitive data objects
Related show

Commit Message

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

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

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

 [v3: Fix kerneldoc errors]

Comments

Linus Torvalds April 7, 2020, 8:09 p.m. UTC | #1
On Tue, Apr 7, 2020 at 1:03 PM Waiman Long <longman@redhat.com> wrote:
>
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.

Ack. Since this isn't exactly high-priority, I'm assuming it will go
through the usual channels (ie Andrew).

             Linus
David Howells April 7, 2020, 8:19 p.m. UTC | #2
Waiman Long <longman@redhat.com> wrote:

> sensitive data objects allocated by kvmalloc(). The relevnat places

"relevant".

>  			if (unlikely(key_data))
> -				__kvzfree(key_data, key_data_len);
> +				kvfree_sensitive(key_data, key_data_len);

I think the if-statement is redundant.

David
David Howells April 7, 2020, 8:21 p.m. UTC | #3
David Howells <dhowells@redhat.com> wrote:

> >  			if (unlikely(key_data))
> > -				__kvzfree(key_data, key_data_len);
> > +				kvfree_sensitive(key_data, key_data_len);
> 
> I think the if-statement is redundant.

Ah - I see that you explicitly wanted to keep it.  There's a good chance it'll
get janitored at some point.

David
Waiman Long April 7, 2020, 8:24 p.m. UTC | #4
On 4/7/20 4:19 PM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> sensitive data objects allocated by kvmalloc(). The relevnat places
> "relevant".

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

Cheers,
Longman
Joe Perches April 7, 2020, 8:31 p.m. UTC | #5
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)
Waiman Long April 7, 2020, 8:45 p.m. UTC | #6
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
Linus Torvalds April 7, 2020, 9:01 p.m. UTC | #7
On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <longman@redhat.com> wrote:
>
> If the memory is really virtually mapped, the only way to find out the
> size of the object is to use find_vm_area() which can be relatively high
> cost and no simple helper function is available.

We _could_ just push it down to a "vfree_sensitive()", and do it
inside the vfree logic. That ends up obviously figuring out the size
of the area eventually.

But since the vmalloc data structures fundamentally aren't irq-safe,
vfree() actually has magical things like "if called in an interrupt,
we'll delay it to work context".

So that "eventually" can be quite a bit later, and it would delay the
overwriting of the sensitive data if we did that.

So this patch does end up simpler, but for vfree data it is actually
technically the better approach too (since overwriting the sensitive
data asap is what you want).

            Linus
Uladzislau Rezki (Sony) April 7, 2020, 9:24 p.m. UTC | #8
On Tue, Apr 07, 2020 at 02:01:01PM -0700, Linus Torvalds wrote:
> On Tue, Apr 7, 2020 at 1:45 PM Waiman Long <longman@redhat.com> wrote:
> >
> > If the memory is really virtually mapped, the only way to find out the
> > size of the object is to use find_vm_area() which can be relatively high
> > cost and no simple helper function is available.
> 
> We _could_ just push it down to a "vfree_sensitive()", and do it
> inside the vfree logic. That ends up obviously figuring out the size
> of the area eventually.
> 
> But since the vmalloc data structures fundamentally aren't irq-safe,
> vfree() actually has magical things like "if called in an interrupt,
> we'll delay it to work context".
> 
Just some thoughts. Sorry for jumping in.

Seems like there is only one place where we can "sleep". I mean when we
call vfree(). That is free_vmap_area_noflush() -> try_purge_vmap_area_lazy().
Basically try_purge_vmap_area_lazy() can call the schedule() what is not 
allowed for IRQs. Instead of inlining the try_purge_vmap_area_lazy()
into current context we can schedule_work(). And i think it makes sense
from many point of views.

Also, we can end up in zeroed non-existance vmap area if we do not find_vmap_area().

Thanks!

--
Vlad Rezki
Linus Torvalds April 7, 2020, 9:30 p.m. UTC | #9
On Tue, Apr 7, 2020 at 2:25 PM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> Seems like there is only one place where we can "sleep". I mean when we
> call vfree(). That is free_vmap_area_noflush() -> try_purge_vmap_area_lazy().
> Basically try_purge_vmap_area_lazy() can call the schedule() what is not
> allowed for IRQs. Instead of inlining the try_purge_vmap_area_lazy()
> into current context we can schedule_work(). And i think it makes sense
> from many point of views.

I don't think that's the only case.

Or rather, that may be the only case of _sleeping_, but we also aren't
irq-safe wrt locking.

And I'm not just talking about the vmap_purge_lock mutex, but all the
spinlocks etc we have.

That said, I haven't looked at that code in _ages_. Maybe those things
would be trivial to just turn into irq-safe ones and there are no real
latency issues anywhere.

                Linus
Matthew Wilcox April 7, 2020, 10:12 p.m. UTC | #10
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.
Joe Perches April 8, 2020, 12:35 a.m. UTC | #11
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.
Jarkko Sakkinen April 8, 2020, 1:38 p.m. UTC | #12
On Tue, Apr 07, 2020 at 04:03:18PM -0400, Waiman Long wrote:
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.
> 
> This patch introduces a new kvfree_sensitive() for freeing those
> sensitive data objects allocated by kvmalloc(). The relevnat places
> where kvfree_sensitive() can be used are modified to use it.
> 
> Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

David, you want to pick this one?

/Jarkko
Eric Biggers May 1, 2020, 11:22 p.m. UTC | #13
On Tue, Apr 07, 2020 at 04:03:18PM -0400, Waiman Long wrote:
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.
> 
> This patch introduces a new kvfree_sensitive() for freeing those
> sensitive data objects allocated by kvmalloc(). The relevnat places
> where kvfree_sensitive() can be used are modified to use it.
> 
> Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Waiman Long <longman@redhat.com>

Looks good, feel free to add:

Reviewed-by: Eric Biggers <ebiggers@google.com>

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

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

- Eric
Waiman Long May 4, 2020, 2:57 a.m. UTC | #14
On 5/1/20 7:22 PM, Eric Biggers wrote:
> On Tue, Apr 07, 2020 at 04:03:18PM -0400, Waiman Long wrote:
>> For kvmalloc'ed data object that contains sensitive information like
>> cryptographic key, we need to make sure that the buffer is always
>> cleared before freeing it. Using memset() alone for buffer clearing may
>> not provide certainty as the compiler may compile it away. To be sure,
>> the special memzero_explicit() has to be used.
>>
>> This patch introduces a new kvfree_sensitive() for freeing those
>> sensitive data objects allocated by kvmalloc(). The relevnat places
>> where kvfree_sensitive() can be used are modified to use it.
>>
>> Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> Looks good, feel free to add:
>
> Reviewed-by: Eric Biggers <ebiggers@google.com>
>
> (I don't really buy the argument that the compiler could compile away memset()
> before kvfree().  But I agree with using memzero_explicit() anyway to make the
> intent explicit.)
>
> I don't see this patch in linux-next yet.  Who is planning to take this patch?
> Presumably David through the keyrings tree, or Andrew through mm?
>
> - Eric
>
Andrew, would you mind taking this patch into the mm-tree?

Thanks,
Longman
Andrew Morton May 5, 2020, 8:35 p.m. UTC | #15
On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <dhowells@redhat.com> wrote:

> David Howells <dhowells@redhat.com> wrote:
> 
> > >  			if (unlikely(key_data))
> > > -				__kvzfree(key_data, key_data_len);
> > > +				kvfree_sensitive(key_data, key_data_len);
> > 
> > I think the if-statement is redundant.
> 
> Ah - I see that you explicitly wanted to keep it.

Why's that?

> There's a good chance it'll get janitored at some point.

Indeed.  Perhaps add a few little comments to explain the reasoning and
to keep the janitorial fingers away?
Waiman Long May 6, 2020, 1:29 a.m. UTC | #16
On 5/5/20 4:35 PM, Andrew Morton wrote:
> On Tue, 07 Apr 2020 21:21:57 +0100 David Howells <dhowells@redhat.com> wrote:
>
>> David Howells <dhowells@redhat.com> wrote:
>>
>>>>   			if (unlikely(key_data))
>>>> -				__kvzfree(key_data, key_data_len);
>>>> +				kvfree_sensitive(key_data, key_data_len);
>>> I think the if-statement is redundant.
>> Ah - I see that you explicitly wanted to keep it.
> Why's that?

There is a comment above it:

                 /*
                  * The key may change (unlikely) in between 2 consecutive
                  * __keyctl_read_key() calls. In this case, we reallocate
                  * a larger buffer and redo the key read when
                  * key_data_len < ret <= buflen.
                  */
                 if (ret > key_data_len) {
                         if (unlikely(key_data))
                                 __kvzfree(key_data, key_data_len);

key_data will be defined only if the unlikely case that the key increase 
in length between the 2 consecutive __keyctl_read_key() call and we have 
to enlarge the buffer and read the key again. I want to keep the 
unlikely() macro to emphasize the fact that this condition should not 
happen.

>> There's a good chance it'll get janitored at some point.
> Indeed.  Perhaps add a few little comments to explain the reasoning and
> to keep the janitorial fingers away?
>
I can reword the comment to make it more explicit and send a v4 if you 
think the current comment is not clear enough.

Cheers,
Longman
Balbir Singh May 14, 2020, 11 a.m. UTC | #17
On 8/4/20 6:03 am, Waiman Long wrote:
> For kvmalloc'ed data object that contains sensitive information like
> cryptographic key, we need to make sure that the buffer is always
> cleared before freeing it. Using memset() alone for buffer clearing may
> not provide certainty as the compiler may compile it away. To be sure,
> the special memzero_explicit() has to be used.
> 
> This patch introduces a new kvfree_sensitive() for freeing those
> sensitive data objects allocated by kvmalloc(). The relevnat places
> where kvfree_sensitive() can be used are modified to use it.
> 
> Fixes: 4f0882491a14 ("KEYS: Avoid false positive ENOMEM error on key read")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  include/linux/mm.h       |  1 +
>  mm/util.c                | 18 ++++++++++++++++++
>  security/keys/internal.h | 11 -----------
>  security/keys/keyctl.c   | 16 +++++-----------
>  4 files changed, 24 insertions(+), 22 deletions(-)
> 
>  [v3: Fix kerneldoc errors]
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7dd5c4ccbf85..9b3130b20f42 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -757,6 +757,7 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
>  }
>  
>  extern void kvfree(const void *addr);
> +extern void kvfree_sensitive(const void *addr, size_t len);
>  
>  static inline int compound_mapcount(struct page *page)
>  {
> diff --git a/mm/util.c b/mm/util.c
> index 988d11e6c17c..dc1c877d5481 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -604,6 +604,24 @@ void kvfree(const void *addr)
>  }
>  EXPORT_SYMBOL(kvfree);
>  
> +/**
> + * kvfree_sensitive - Free a data object containing sensitive information.
> + * @addr: address of the data object to be freed.
> + * @len: length of the data object.
> + *
> + * Use the special memzero_explicit() function to clear the content of a
> + * kvmalloc'ed object containing sensitive data to make sure that the
> + * compiler won't optimize out the data clearing.
> + */
> +void kvfree_sensitive(const void *addr, size_t len)
> +{
> +	if (likely(!ZERO_OR_NULL_PTR(addr))) {
> +		memzero_explicit((void *)addr, len);
> +		kvfree(addr);
> +	}
> +}
> +EXPORT_SYMBOL(kvfree_sensitive);
> +

I wonder if the right thing to do is also to disable pre-emption, just so that the thread does not linger on with sensitive data.

void kvfree_sensitive(const void *addr, size_t len)
{
	preempt_disable();
	if (likely(!ZERO_OR_NULL_PTR(addr))) {
		memzero_explicit((void *)addr, len);
		kvfree(addr);
	}
	preempt_enable();
}
EXPORT_SYMBOL(kvfree_sensitive);



Balbir Singh.
Matthew Wilcox May 14, 2020, noon UTC | #18
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.
Joe Perches May 14, 2020, 12:08 p.m. UTC | #19
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 ?
Balbir Singh May 17, 2020, 12:27 a.m. UTC | #20
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.
Matthew Wilcox May 17, 2020, 12:44 a.m. UTC | #21
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?
Waiman Long May 18, 2020, 2:39 a.m. UTC | #22
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

Patch
diff mbox series

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