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