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

Jia-Ju Bai <baijiaju1990@xxxxxxxxx> · Thu, 25 Jul 2019 11:33:53 +0800

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