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