Re: [bug report] crypto: aesni - xts_crypt() return if walk.nbytes is 0

On Sat, Sep 11, 2021 at 06:23:04PM +0200, Ard Biesheuvel wrote:

On Sat, 11 Sept 2021 at 09:32, Dan Carpenter [off-list ref] wrote:

quoted

On Fri, Sep 10, 2021 at 09:27:46PM +0530, Shreyansh Chouhan wrote:

quoted

On Fri, Sep 10, 2021 at 09:24:37PM +0530, Shreyansh Chouhan wrote:

quoted

Hi Dan,

Sorry for the delay in the response.

On Wed, Sep 08, 2021 at 03:54:40PM +0300, Dan Carpenter wrote:

quoted

Hello Shreyansh Chouhan,

The patch 72ff2bf04db2: "crypto: aesni - xts_crypt() return if
walk.nbytes is 0" from Aug 22, 2021, leads to the following
Smatch static checker warning:

  arch/x86/crypto/aesni-intel_glue.c:915 xts_crypt()
  warn: possible missing kernel_fpu_end()

arch/x86/crypto/aesni-intel_glue.c
    839 static int xts_crypt(struct skcipher_request *req, bool encrypt)
    840 {
    841         struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
    842         struct aesni_xts_ctx *ctx = crypto_skcipher_ctx(tfm);
    843         int tail = req->cryptlen % AES_BLOCK_SIZE;
    844         struct skcipher_request subreq;
    845         struct skcipher_walk walk;
    846         int err;
    847
    848         if (req->cryptlen < AES_BLOCK_SIZE)
    849                 return -EINVAL;
    850
    851         err = skcipher_walk_virt(&walk, req, false);
    852         if (!walk.nbytes)
    853                 return err;

The patch adds this check for "walk.nbytes == 0".

    854
    855         if (unlikely(tail > 0 && walk.nbytes < walk.total)) {
                                         ^^^^^^^^^^^^^^^^^^^^^^^^
But Smatch says that "walk.nbytes" can be set to zero inside this
if statement.

Indeed that is so, I missed it the first time around.

quoted

    856                 int blocks = DIV_ROUND_UP(req->cryptlen, AES_BLOCK_SIZE) - 2;
    857
    858                 skcipher_walk_abort(&walk);
    859
    860                 skcipher_request_set_tfm(&subreq, tfm);
    861                 skcipher_request_set_callback(&subreq,
    862                                               skcipher_request_flags(req),
    863                                               NULL, NULL);
    864                 skcipher_request_set_crypt(&subreq, req->src, req->dst,
    865                                            blocks * AES_BLOCK_SIZE, req->iv);
    866                 req = &subreq;
    867
    868                 err = skcipher_walk_virt(&walk, req, false);
    869                 if (err)
    870                         return err;

We can replace the above if (err) check with another if
(!walk.nbytes) check.

quoted

    871         } else {
    872                 tail = 0;
    873         }
    874
    875         kernel_fpu_begin();
    876
    877         /* calculate first value of T */
    878         aesni_enc(aes_ctx(ctx->raw_tweak_ctx), walk.iv, walk.iv);
    879

Leading to not entering this loop and so we don't restore kernel_fpu_end().

So maybe the "if (walk.nbytes == 0)" check should be moved to right
before the call to kernel_fpu_begin()?

Instead of moving the first walk.nbytes check, I think we can have two if
(!walk.nbytes) checks. There was a discussion between Herbert Xu and Ard
Biesheuvel, and Herbert wrote in his email that most skcipher walkers are
not supposed to explicitly check on the err value, and should instead
terminate the loop whenever walk.nbytes is set to 0.

Here is a link to that discussion:

https://lore.kernel.org/linux-crypto/20210820125315.GB28484@gondor.apana.org.au/ (local)

I can send in a patch that replaces the if (err) check with an if
(!walk.nbytes) check if that is fine with you.

Yes, please!

Ehm, how would that patch be any different from the one that you sent
2+ weeks ago and which was already merged by Herbert and pulled by
Linus?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72ff2bf04db2a48840df93a461b7115900f46c05

The previous patch that I sent adds a walk.nbytes after the
skcipher_walk_virt() call at line 851[1] in the code. There is another
call to skcipher_walk_virt() before we call kernel_fpu_begin(), at line
868[2], this patch adds a walk.nbytes check after that function call.

Regards,
Shreyansh Chouhan

--
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/x86/crypto/aesni-intel_glue.c#n851
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/x86/crypto/aesni-intel_glue.c#n868