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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux