| Message ID | 20200911114400.82207-1-alex.dewar90@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | security: keys: Use kvfree_sensitive in a few places | expand |
On Fri, 2020-09-11 at 12:44 +0100, Alex Dewar wrote: > In big_key.c, there are a few places where memzero_explicit + kvfree > is used. It is better to use kvfree_sensitive instead, which is more > readable and also prevents the compiler from eliding the call to > memzero_explicit. Fix this. That last bit is untrue: the compiler can't elide memzero_explicit ... that's why it has the explicit suffix. The original problem was a lot of people do memset(.., 0, ..); kfree() which the compiler can elide if it understands the memory is going out of scope. Or the even more problematic memset(..., 0, ...) on a stack variable before it goes out of scope. We can argue about readability but there's no secret leak here. James
On Fri, Sep 11, 2020 at 08:04:24AM -0700, James Bottomley wrote: > On Fri, 2020-09-11 at 12:44 +0100, Alex Dewar wrote: > > In big_key.c, there are a few places where memzero_explicit + kvfree > > is used. It is better to use kvfree_sensitive instead, which is more > > readable and also prevents the compiler from eliding the call to > > memzero_explicit. Fix this. > > That last bit is untrue: the compiler can't elide memzero_explicit ... > that's why it has the explicit suffix. > > The original problem was a lot of people do memset(.., 0, ..); kfree() > which the compiler can elide if it understands the memory is going out > of scope. Or the even more problematic memset(..., 0, ...) on a stack > variable before it goes out of scope. > > We can argue about readability but there's no secret leak here. Ahh, my mistake. Thanks for the explanation. I'll send a v2 with an updated commit message. I think it would still make sense to use kfree_sensitive here as on next-20200911 this is the last use of kzfree in the tree and it would be nice to excise it altogether. Best, Alex > > James >
On Fri, Sep 11, 2020 at 05:00:09PM +0100, Alex Dewar wrote: > On Fri, Sep 11, 2020 at 08:04:24AM -0700, James Bottomley wrote: > > On Fri, 2020-09-11 at 12:44 +0100, Alex Dewar wrote: > > > In big_key.c, there are a few places where memzero_explicit + kvfree > > > is used. It is better to use kvfree_sensitive instead, which is more > > > readable and also prevents the compiler from eliding the call to > > > memzero_explicit. Fix this. > > > > That last bit is untrue: the compiler can't elide memzero_explicit ... > > that's why it has the explicit suffix. > > > > The original problem was a lot of people do memset(.., 0, ..); kfree() > > which the compiler can elide if it understands the memory is going out > > of scope. Or the even more problematic memset(..., 0, ...) on a stack > > variable before it goes out of scope. > > > > We can argue about readability but there's no secret leak here. > > Ahh, my mistake. Thanks for the explanation. > > I'll send a v2 with an updated commit message. I think it would still > make sense to use kfree_sensitive here as on next-20200911 this is the > last use of kzfree in the tree and it would be nice to excise it > altogether. Ignore this! I thought we were talking about a different patch :-/ I'll send a respin with a better commit message anyways. Cheers :-) > > Best, > Alex > > > > > James > >
Hi, same patch https://lkml.org/lkml/2020/8/27/168 Thanks, Denis On 9/11/20 2:44 PM, Alex Dewar wrote: > In big_key.c, there are a few places where memzero_explicit + kvfree is > used. It is better to use kvfree_sensitive instead, which is more > readable and also prevents the compiler from eliding the call to > memzero_explicit. Fix this. > > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com> > --- > security/keys/big_key.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/security/keys/big_key.c b/security/keys/big_key.c > index 691347dea3c1..d17e5f09eeb8 100644 > --- a/security/keys/big_key.c > +++ b/security/keys/big_key.c > @@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) > *path = file->f_path; > path_get(path); > fput(file); > - memzero_explicit(buf, enclen); > - kvfree(buf); > + kvfree_sensitive(buf, enclen); > } else { > /* Just store the data in a buffer */ > void *data = kmalloc(datalen, GFP_KERNEL); > @@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) > err_enckey: > kfree_sensitive(enckey); > error: > - memzero_explicit(buf, enclen); > - kvfree(buf); > + kvfree_sensitive(buf, enclen); > return ret; > } > > @@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen) > err_fput: > fput(file); > error: > - memzero_explicit(buf, enclen); > - kvfree(buf); > + kvfree_sensitive(buf, enclen); > } else { > ret = datalen; > memcpy(buffer, key->payload.data[big_key_data], datalen); >
On 2020-09-11 17:05, Denis Efremov wrote: > Hi, > > same patch > > https://lkml.org/lkml/2020/8/27/168 > > Thanks, > Denis Ah ok. Sorry for the noise! > > On 9/11/20 2:44 PM, Alex Dewar wrote: >> In big_key.c, there are a few places where memzero_explicit + kvfree is >> used. It is better to use kvfree_sensitive instead, which is more >> readable and also prevents the compiler from eliding the call to >> memzero_explicit. Fix this. >> >> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com> >> --- >> security/keys/big_key.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/security/keys/big_key.c b/security/keys/big_key.c >> index 691347dea3c1..d17e5f09eeb8 100644 >> --- a/security/keys/big_key.c >> +++ b/security/keys/big_key.c >> @@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) >> *path = file->f_path; >> path_get(path); >> fput(file); >> - memzero_explicit(buf, enclen); >> - kvfree(buf); >> + kvfree_sensitive(buf, enclen); >> } else { >> /* Just store the data in a buffer */ >> void *data = kmalloc(datalen, GFP_KERNEL); >> @@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) >> err_enckey: >> kfree_sensitive(enckey); >> error: >> - memzero_explicit(buf, enclen); >> - kvfree(buf); >> + kvfree_sensitive(buf, enclen); >> return ret; >> } >> >> @@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen) >> err_fput: >> fput(file); >> error: >> - memzero_explicit(buf, enclen); >> - kvfree(buf); >> + kvfree_sensitive(buf, enclen); >> } else { >> ret = datalen; >> memcpy(buffer, key->payload.data[big_key_data], datalen); >>
On Fri, Sep 11, 2020 at 07:05:16PM +0300, Denis Efremov wrote: > Hi, > > same patch > > https://lkml.org/lkml/2020/8/27/168 > > Thanks, > Denis David, can you pick this up? /Jarkko
diff --git a/security/keys/big_key.c b/security/keys/big_key.c index 691347dea3c1..d17e5f09eeb8 100644 --- a/security/keys/big_key.c +++ b/security/keys/big_key.c @@ -121,8 +121,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) *path = file->f_path; path_get(path); fput(file); - memzero_explicit(buf, enclen); - kvfree(buf); + kvfree_sensitive(buf, enclen); } else { /* Just store the data in a buffer */ void *data = kmalloc(datalen, GFP_KERNEL); @@ -140,8 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep) err_enckey: kfree_sensitive(enckey); error: - memzero_explicit(buf, enclen); - kvfree(buf); + kvfree_sensitive(buf, enclen); return ret; } @@ -273,8 +271,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen) err_fput: fput(file); error: - memzero_explicit(buf, enclen); - kvfree(buf); + kvfree_sensitive(buf, enclen); } else { ret = datalen; memcpy(buffer, key->payload.data[big_key_data], datalen);
In big_key.c, there are a few places where memzero_explicit + kvfree is used. It is better to use kvfree_sensitive instead, which is more readable and also prevents the compiler from eliding the call to memzero_explicit. Fix this. Signed-off-by: Alex Dewar <alex.dewar90@gmail.com> --- security/keys/big_key.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)