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
