Re: potential underfow in crypto/lrw.c setkey() setkey

[Eric Biggers <ebiggers@kernel.org>]

On Thu, May 09, 2019 at 02:06:22PM +0300, Dan Carpenter wrote:
> crypto/lrw.c
>     72  static int setkey(struct crypto_skcipher *parent, const u8 *key,
>     73                    unsigned int keylen)
>     74  {
>     75          struct priv *ctx = crypto_skcipher_ctx(parent);
>     76          struct crypto_skcipher *child = ctx->child;
>     77          int err, bsize = LRW_BLOCK_SIZE;
>     78          const u8 *tweak = key + keylen - bsize;
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Smatch thinks that keylen is user controlled from zero to some upper
> bound.  How do we know it's >= LRW_BLOCK_SIZE (16)?

See create() in crypto/lrw.c, which is the function that creates an LRW
instance.  It sets .min_keysize and .max_keysize:

        inst->alg.min_keysize = crypto_skcipher_alg_min_keysize(alg) +
                                LRW_BLOCK_SIZE;
        inst->alg.max_keysize = crypto_skcipher_alg_max_keysize(alg) +
                                LRW_BLOCK_SIZE;

Then when sometime calls crypto_skcipher_setkey(), it calls skcipher_setkey() in
crypto/skcipher.c which verifies the key length is in within the bounds the
algorithm declares:

        if (keylen < cipher->min_keysize || keylen > cipher->max_keysize) {
                crypto_skcipher_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
                return -EINVAL;
        }

> 
> I find the crypto code sort of hard to follow...  There are a bunch of
> setkey pointers and they're sometimes called recursively.  Is there
> some trick or hints?
> 
>     79          be128 tmp = { 0 };
>     80          int i;
>     81  
>     82          crypto_skcipher_clear_flags(child, CRYPTO_TFM_REQ_MASK);
>     83          crypto_skcipher_set_flags(child, crypto_skcipher_get_flags(parent) &
>     84                                           CRYPTO_TFM_REQ_MASK);
>     85          err = crypto_skcipher_setkey(child, key, keylen - bsize);
>     86          crypto_skcipher_set_flags(parent, crypto_skcipher_get_flags(child) &
>     87                                            CRYPTO_TFM_RES_MASK);
>     88          if (err)
>     89                  return err;

LRW is a template for a block cipher mode of operation which is implemented on
top of ECB.  So, LRW's setkey() method calls setkey() on the underlying ECB
transform.  Similarly, ECB then may call setkey() of the underlying block cipher
such as AES, or alternatively it may be implemented directly.

- Eric