Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 21:24:32 +0530
From:   Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     linux-crypto@vger.kernel.org
Subject: Re: [bug report] crypto: aesni - xts_crypt() return if walk.nbytes
 is 0
Message-ID: <YTt/uJjgy4jTr+GL@fedora>
References: <20210908125440.GA6147@kili>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210908125440.GA6147@kili>
Precedence: bulk

Hi Dan,

Sorry for the delay in the response.

On Wed, Sep 08, 2021 at 03:54:40PM +0300, Dan Carpenter wrote:
> 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.

>     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.

>     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/

>     880         while (walk.nbytes > 0) {
>     881                 int nbytes = walk.nbytes;
>     882 
>     883                 if (nbytes < walk.total)
>     884                         nbytes &= ~(AES_BLOCK_SIZE - 1);
>     885 
>     886                 if (encrypt)
>     887                         aesni_xts_encrypt(aes_ctx(ctx->raw_crypt_ctx),
>     888                                           walk.dst.virt.addr, walk.src.virt.addr,
>     889                                           nbytes, walk.iv);
>     890                 else
>     891                         aesni_xts_decrypt(aes_ctx(ctx->raw_crypt_ctx),
>     892                                           walk.dst.virt.addr, walk.src.virt.addr,
>     893                                           nbytes, walk.iv);
>     894                 kernel_fpu_end();
>     895 
>     896                 err = skcipher_walk_done(&walk, walk.nbytes - nbytes);
>     897 
>     898                 if (walk.nbytes > 0)
>     899                         kernel_fpu_begin();
>     900         }
>     901 
>     902         if (unlikely(tail > 0 && !err)) {
>     903                 struct scatterlist sg_src[2], sg_dst[2];
>     904                 struct scatterlist *src, *dst;
>     905 
>     906                 dst = src = scatterwalk_ffwd(sg_src, req->src, req->cryptlen);
>     907                 if (req->dst != req->src)
>     908                         dst = scatterwalk_ffwd(sg_dst, req->dst, req->cryptlen);
>     909 
>     910                 skcipher_request_set_crypt(req, src, dst, AES_BLOCK_SIZE + tail,
>     911                                            req->iv);
>     912 
>     913                 err = skcipher_walk_virt(&walk, &subreq, false);
>     914                 if (err)
> --> 915                         return err;
>     916 
>     917                 kernel_fpu_begin();
>     918                 if (encrypt)
>     919                         aesni_xts_encrypt(aes_ctx(ctx->raw_crypt_ctx),
>     920                                           walk.dst.virt.addr, walk.src.virt.addr,
>     921                                           walk.nbytes, walk.iv);
>     922                 else
>     923                         aesni_xts_decrypt(aes_ctx(ctx->raw_crypt_ctx),
>     924                                           walk.dst.virt.addr, walk.src.virt.addr,
>     925                                           walk.nbytes, walk.iv);
>     926                 kernel_fpu_end();
>     927 
>     928                 err = skcipher_walk_done(&walk, 0);
>     929         }
>     930         return err;
>     931 }
> 
> regards,
> dan carpenter