Re: [PATCH] fs: crypto: keyinfo: Fix a possible null-pointer dereference in derive_key_aes()

[Jia-Ju Bai]

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