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:27:46 +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: <YTuAerH3S78Xf1Jc@fedora>
References: <20210908125440.GA6147@kili>
 <YTt/uJjgy4jTr+GL@fedora>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YTt/uJjgy4jTr+GL@fedora>
Precedence: bulk

On Fri, Sep 10, 2021 at 09:24:37PM +0530, Shreyansh Chouhan wrote:
> 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/
> 

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

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

Rehards,
Shreyansh Chouhan