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

[Jia-Ju Bai]

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