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.
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;
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()?
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
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/[email protected]/
> 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
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/[email protected]/
>
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
On Fri, Sep 10, 2021 at 09:27:46PM +0530, Shreyansh Chouhan wrote:
> 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/[email protected]/
> >
>
> 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!
regards,
dan carpenter
On Sat, 11 Sept 2021 at 09:32, Dan Carpenter <[email protected]> wrote:
>
> On Fri, Sep 10, 2021 at 09:27:46PM +0530, Shreyansh Chouhan wrote:
> > 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/[email protected]/
> > >
> >
> > 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
On Sat, Sep 11, 2021 at 06:23:04PM +0200, Ard Biesheuvel wrote:
> On Sat, 11 Sept 2021 at 09:32, Dan Carpenter <[email protected]> wrote:
> >
> > On Fri, Sep 10, 2021 at 09:27:46PM +0530, Shreyansh Chouhan wrote:
> > > 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/[email protected]/
> > > >
> > >
> > > 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
On Sun, 12 Sept 2021 at 07:02, Shreyansh Chouhan
<[email protected]> wrote:
>
> On Sat, Sep 11, 2021 at 06:23:04PM +0200, Ard Biesheuvel wrote:
> > On Sat, 11 Sept 2021 at 09:32, Dan Carpenter <[email protected]> wrote:
> > >
> > > On Fri, Sep 10, 2021 at 09:27:46PM +0530, Shreyansh Chouhan wrote:
> > > > 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/[email protected]/
> > > > >
> > > >
> > > > 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.
>
Ah, ok. Thanks for pointing that out.
So while the first occurrence can actually be triggered by fuzzing
(even though having zero length inputs does not make sense for a
skcipher invocation), this second occurrence can only be triggered if
the request changes size under our feet, i.e., from >0 to 0.
I'll leave it up to Herbert to decide if we want to make this change
anyway, but IMO, we don't need it.