* [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
@ 2019-07-24 10:02 Jia-Ju Bai
2019-07-24 16:07 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2019-07-24 10:02 UTC (permalink / raw)
To: tytso, jaegeuk, ebiggers; +Cc: linux-fscrypt, linux-kernel, Jia-Ju Bai
In derive_key_aes(), tfm is assigned to NULL on line 46, and then
crypto_free_skcipher(tfm) is executed.
crypto_free_skcipher(tfm)
crypto_skcipher_tfm(tfm)
return &tfm->base;
Thus, a possible null-pointer dereference may occur.
To fix this bug, tfm is checked before calling crypto_free_skcipher().
This bug is found by a static analysis tool STCheck written by us.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
fs/crypto/keyinfo.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 207ebed918c1..b419720cac54 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -66,7 +66,8 @@ static int derive_key_aes(const u8 *master_key,
res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
out:
skcipher_request_free(req);
- crypto_free_skcipher(tfm);
+ if (tfm)
+ crypto_free_skcipher(tfm);
return res;
}
--
2.17.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
2019-07-24 10:02 [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes() Jia-Ju Bai
@ 2019-07-24 16:07 ` Eric Biggers
2019-07-25 3:30 ` Jia-Ju Bai
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2019-07-24 16:07 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: tytso, jaegeuk, linux-fscrypt, linux-kernel, linux-crypto
[+Cc linux-crypto]
On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote:
> In derive_key_aes(), tfm is assigned to NULL on line 46, and then
> crypto_free_skcipher(tfm) is executed.
>
> crypto_free_skcipher(tfm)
> crypto_skcipher_tfm(tfm)
> return &tfm->base;
>
> Thus, a possible null-pointer dereference may occur.
This analysis is incorrect because only the address &tfm->base is taken.
There's no pointer dereference.
In fact all the crypto_free_*() functions are no-ops on NULL pointers, and many
other callers rely on it. So there's no bug here.
It appears you've sent the same patch for some of these other callers
(https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22), but none
are Cc'ed to linux-crypto or another mailing list I'm subscribed to, so I can't
respond to them. But this feedback applies equally to them too.
Note also that if there actually were a bug here (which again, there doesn't
appear to be), we'd need to fix it in crypto_free_*(), not in the callers.
- Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
2019-07-24 16:07 ` Eric Biggers
@ 2019-07-25 3:30 ` Jia-Ju Bai
2019-07-25 3:33 ` Jia-Ju Bai
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2019-07-25 3:30 UTC (permalink / raw)
To: tytso, jaegeuk, linux-fscrypt, linux-kernel, linux-crypto
On 2019/7/25 0:07, Eric Biggers wrote:
> [+Cc linux-crypto]
>
> On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote:
>> In derive_key_aes(), tfm is assigned to NULL on line 46, and then
>> crypto_free_skcipher(tfm) is executed.
>>
>> crypto_free_skcipher(tfm)
>> crypto_skcipher_tfm(tfm)
>> return &tfm->base;
>>
>> Thus, a possible null-pointer dereference may occur.
> This analysis is incorrect because only the address &tfm->base is taken.
> There's no pointer dereference.
>
> In fact all the crypto_free_*() functions are no-ops on NULL pointers, and many
> other callers rely on it. So there's no bug here.
Thanks for the reply :)
I admit that "&tfm->base" is not a null-pointer dereference when tfm is
NULL.
But I still think crypto_free_skcipher(tfm) can cause security problems
when tfm is NULL.
Looking at the code:
static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
{
crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
}
static inline struct crypto_tfm *crypto_skcipher_tfm(
struct crypto_skcipher *tfm)
{
return &tfm->base;
}
void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
{
struct crypto_alg *alg;
if (unlikely(!mem))
return;
alg = tfm->__crt_alg;
if (!tfm->exit && alg->cra_exit)
alg->cra_exit(tfm);
crypto_exit_ops(tfm);
crypto_mod_put(alg);
kzfree(mem);
}
The function crypto_skcipher_tfm() may return an uninitialized address
(&tfm->base) when tfm is NULL.
Then crypto_destroy_tfm() uses this problematic address (tfm), which may
cause security problems.
Besides, I also find that some kernel modules check tfm before calling
crypto_free_*(), such as:
drivers/crypto/vmx/aes_xts.c:
if (ctx->fallback) {
crypto_free_skcipher(ctx->fallback);
ctx->fallback = NULL;
}
net/rxrpc/rxkad.c:
if (conn->cipher)
crypto_free_skcipher(conn->cipher);
drivers/crypto/chelsio/chcr_algo.c:
if (ablkctx->aes_generic)
crypto_free_cipher(ablkctx->aes_generic);
net/mac80211/wep.c:
if (!IS_ERR(local->wep_tx_tfm))
crypto_free_cipher(local->wep_tx_tfm);
Thus, I think it is better to check tfm before calling crypto_free_*().
>
> It appears you've sent the same patch for some of these other callers
> (https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22), but none
> are Cc'ed to linux-crypto or another mailing list I'm subscribed to, so I can't
> respond to them. But this feedback applies equally to them too.
Ah, sorry.
I just ran "get_maintainer.pl" for the kernel modules used
crypto_free_*(), and forgot to cc to linux-crypto...
>
> Note also that if there actually were a bug here (which again, there doesn't
> appear to be), we'd need to fix it in crypto_free_*(), not in the callers.
>
I think a possible way is to add a check of tfm in crypto_free_*(), such as:
static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
{
if (tfm)
crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
}
If you think it is okay, I can send a patch for this.
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
2019-07-25 3:30 ` Jia-Ju Bai
@ 2019-07-25 3:33 ` Jia-Ju Bai
2019-07-25 3:39 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2019-07-25 3:33 UTC (permalink / raw)
To: tytso, jaegeuk, ebiggers; +Cc: linux-fscrypt, linux-kernel, linux-crypto
Sorry, I forgot to send to Eric, so send it again.
On 2019/7/25 11:30, Jia-Ju Bai wrote:
>
>
> On 2019/7/25 0:07, Eric Biggers wrote:
>> [+Cc linux-crypto]
>>
>> On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote:
>>> In derive_key_aes(), tfm is assigned to NULL on line 46, and then
>>> crypto_free_skcipher(tfm) is executed.
>>>
>>> crypto_free_skcipher(tfm)
>>> crypto_skcipher_tfm(tfm)
>>> return &tfm->base;
>>>
>>> Thus, a possible null-pointer dereference may occur.
>> This analysis is incorrect because only the address &tfm->base is taken.
>> There's no pointer dereference.
>>
>> In fact all the crypto_free_*() functions are no-ops on NULL
>> pointers, and many
>> other callers rely on it. So there's no bug here.
>
> Thanks for the reply :)
> I admit that "&tfm->base" is not a null-pointer dereference when tfm
> is NULL.
> But I still think crypto_free_skcipher(tfm) can cause security
> problems when tfm is NULL.
>
> Looking at the code:
>
> static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
> {
> crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
> }
>
> static inline struct crypto_tfm *crypto_skcipher_tfm(
> struct crypto_skcipher *tfm)
> {
> return &tfm->base;
> }
>
> void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
> {
> struct crypto_alg *alg;
>
> if (unlikely(!mem))
> return;
>
> alg = tfm->__crt_alg;
>
> if (!tfm->exit && alg->cra_exit)
> alg->cra_exit(tfm);
> crypto_exit_ops(tfm);
> crypto_mod_put(alg);
> kzfree(mem);
> }
>
> The function crypto_skcipher_tfm() may return an uninitialized address
> (&tfm->base) when tfm is NULL.
> Then crypto_destroy_tfm() uses this problematic address (tfm), which
> may cause security problems.
>
> Besides, I also find that some kernel modules check tfm before calling
> crypto_free_*(), such as:
>
> drivers/crypto/vmx/aes_xts.c:
> if (ctx->fallback) {
> crypto_free_skcipher(ctx->fallback);
> ctx->fallback = NULL;
> }
>
> net/rxrpc/rxkad.c:
> if (conn->cipher)
> crypto_free_skcipher(conn->cipher);
>
> drivers/crypto/chelsio/chcr_algo.c:
> if (ablkctx->aes_generic)
> crypto_free_cipher(ablkctx->aes_generic);
>
> net/mac80211/wep.c:
> if (!IS_ERR(local->wep_tx_tfm))
> crypto_free_cipher(local->wep_tx_tfm);
>
> Thus, I think it is better to check tfm before calling crypto_free_*().
>
>>
>> It appears you've sent the same patch for some of these other callers
>> (https://lore.kernel.org/lkml/?q=%22fix+a+possible+null-pointer%22),
>> but none
>> are Cc'ed to linux-crypto or another mailing list I'm subscribed to,
>> so I can't
>> respond to them. But this feedback applies equally to them too.
>
> Ah, sorry.
> I just ran "get_maintainer.pl" for the kernel modules used
> crypto_free_*(), and forgot to cc to linux-crypto...
>
>>
>> Note also that if there actually were a bug here (which again, there
>> doesn't
>> appear to be), we'd need to fix it in crypto_free_*(), not in the
>> callers.
>>
>
> I think a possible way is to add a check of tfm in crypto_free_*(),
> such as:
> static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
> {
> if (tfm)
> crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
> }
>
> If you think it is okay, I can send a patch for this.
>
>
> Best wishes,
> Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
2019-07-25 3:33 ` Jia-Ju Bai
@ 2019-07-25 3:39 ` Eric Biggers
2019-07-25 3:52 ` Jia-Ju Bai
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2019-07-25 3:39 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: tytso, jaegeuk, linux-fscrypt, linux-kernel, linux-crypto
On Thu, Jul 25, 2019 at 11:33:53AM +0800, Jia-Ju Bai wrote:
> Sorry, I forgot to send to Eric, so send it again.
>
> On 2019/7/25 11:30, Jia-Ju Bai wrote:
> >
> >
> > On 2019/7/25 0:07, Eric Biggers wrote:
> > > [+Cc linux-crypto]
> > >
> > > On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote:
> > > > In derive_key_aes(), tfm is assigned to NULL on line 46, and then
> > > > crypto_free_skcipher(tfm) is executed.
> > > >
> > > > crypto_free_skcipher(tfm)
> > > > ���� crypto_skcipher_tfm(tfm)
> > > > �������� return &tfm->base;
> > > >
> > > > Thus, a possible null-pointer dereference may occur.
> > > This analysis is incorrect because only the address &tfm->base is taken.
> > > There's no pointer dereference.
> > >
> > > In fact all the crypto_free_*() functions are no-ops on NULL
> > > pointers, and many
> > > other callers rely on it.� So there's no bug here.
> >
> > Thanks for the reply :)
> > I admit that "&tfm->base" is not a null-pointer dereference when tfm is
> > NULL.
> > But I still think crypto_free_skcipher(tfm) can cause security problems
> > when tfm is NULL.
> >
> > Looking at the code:
> >
> > static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
> > {
> > ��� crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
> > }
> >
> > static inline struct crypto_tfm *crypto_skcipher_tfm(
> > ��� struct crypto_skcipher *tfm)
> > {
> > ��� return &tfm->base;
> > }
> >
> > void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
> > {
> > ��� struct crypto_alg *alg;
> >
> > ��� if (unlikely(!mem))
> > ��� ��� return;
When the original pointer is NULL, mem == NULL here so crypto_destroy_tfm() is a
no-op.
> > Besides, I also find that some kernel modules check tfm before calling
> > crypto_free_*(), such as:
> >
> > drivers/crypto/vmx/aes_xts.c:
> > ��� if (ctx->fallback) {
> > ��� ��� crypto_free_skcipher(ctx->fallback);
> > ��� ��� ctx->fallback = NULL;
> > ��� }
> >
> > net/rxrpc/rxkad.c:
> > ��� if (conn->cipher)
> > ��� ��� crypto_free_skcipher(conn->cipher);
> >
> > drivers/crypto/chelsio/chcr_algo.c:
> > ��� if (ablkctx->aes_generic)
> > ��� ��� crypto_free_cipher(ablkctx->aes_generic);
> >
> > net/mac80211/wep.c:
> > ��� if (!IS_ERR(local->wep_tx_tfm))
> > ��� ��� crypto_free_cipher(local->wep_tx_tfm);
> >
Well, people sometimes do that for kfree() too. But that doesn't mean it's
needed, or that it's the preferred style (it's not).
- Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()
2019-07-25 3:39 ` Eric Biggers
@ 2019-07-25 3:52 ` Jia-Ju Bai
0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2019-07-25 3:52 UTC (permalink / raw)
To: tytso, jaegeuk, ebiggers; +Cc: linux-fscrypt, linux-kernel, linux-crypto
On 2019/7/25 11:39, Eric Biggers wrote:
> On Thu, Jul 25, 2019 at 11:33:53AM +0800, Jia-Ju Bai wrote:
>> Sorry, I forgot to send to Eric, so send it again.
>>
>> On 2019/7/25 11:30, Jia-Ju Bai wrote:
>>>
>>> On 2019/7/25 0:07, Eric Biggers wrote:
>>>> [+Cc linux-crypto]
>>>>
>>>> On Wed, Jul 24, 2019 at 06:02:04PM +0800, Jia-Ju Bai wrote:
>>>>> In derive_key_aes(), tfm is assigned to NULL on line 46, and then
>>>>> crypto_free_skcipher(tfm) is executed.
>>>>>
>>>>> crypto_free_skcipher(tfm)
>>>>> crypto_skcipher_tfm(tfm)
>>>>> return &tfm->base;
>>>>>
>>>>> Thus, a possible null-pointer dereference may occur.
>>>> This analysis is incorrect because only the address &tfm->base is taken.
>>>> There's no pointer dereference.
>>>>
>>>> In fact all the crypto_free_*() functions are no-ops on NULL
>>>> pointers, and many
>>>> other callers rely on it. So there's no bug here.
>>> Thanks for the reply :)
>>> I admit that "&tfm->base" is not a null-pointer dereference when tfm is
>>> NULL.
>>> But I still think crypto_free_skcipher(tfm) can cause security problems
>>> when tfm is NULL.
>>>
>>> Looking at the code:
>>>
>>> static inline void crypto_free_skcipher(struct crypto_skcipher *tfm)
>>> {
>>> crypto_destroy_tfm(tfm, crypto_skcipher_tfm(tfm));
>>> }
>>>
>>> static inline struct crypto_tfm *crypto_skcipher_tfm(
>>> struct crypto_skcipher *tfm)
>>> {
>>> return &tfm->base;
>>> }
>>>
>>> void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
>>> {
>>> struct crypto_alg *alg;
>>>
>>> if (unlikely(!mem))
>>> return;
> When the original pointer is NULL, mem == NULL here so crypto_destroy_tfm() is a
> no-op.
I overlooked this if statement, thanks for the pointer.
>
>>> Besides, I also find that some kernel modules check tfm before calling
>>> crypto_free_*(), such as:
>>>
>>> drivers/crypto/vmx/aes_xts.c:
>>> if (ctx->fallback) {
>>> crypto_free_skcipher(ctx->fallback);
>>> ctx->fallback = NULL;
>>> }
>>>
>>> net/rxrpc/rxkad.c:
>>> if (conn->cipher)
>>> crypto_free_skcipher(conn->cipher);
>>>
>>> drivers/crypto/chelsio/chcr_algo.c:
>>> if (ablkctx->aes_generic)
>>> crypto_free_cipher(ablkctx->aes_generic);
>>>
>>> net/mac80211/wep.c:
>>> if (!IS_ERR(local->wep_tx_tfm))
>>> crypto_free_cipher(local->wep_tx_tfm);
>>>
> Well, people sometimes do that for kfree() too. But that doesn't mean it's
> needed, or that it's the preferred style (it's not).
Okay.
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-25 3:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 10:02 [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes() Jia-Ju Bai
2019-07-24 16:07 ` Eric Biggers
2019-07-25 3:30 ` Jia-Ju Bai
2019-07-25 3:33 ` Jia-Ju Bai
2019-07-25 3:39 ` Eric Biggers
2019-07-25 3:52 ` Jia-Ju Bai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).