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
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?
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.
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 ?
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.
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
> Signed-off-by: Waiman Long
> ---
> 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.
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 wrote:
David Howells 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
Re: [PATCH v3] mm: Add kvfree_sensitive() for freeing sensitive data objects
On Tue, 07 Apr 2020 21:21:57 +0100 David Howells wrote: > David Howells 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?
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
Signed-off-by: Waiman Long
Looks good, feel free to add:
Reviewed-by: Eric Biggers
(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
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
> Signed-off-by: Waiman Long
Looks good, feel free to add:
Reviewed-by: Eric Biggers
(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
