From: Ard Biesheuvel <ardb@kernel.org>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>,
Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Subject: Re: [bug report] crypto: aesni - xts_crypt() return if walk.nbytes is 0
Date: Sat, 11 Sep 2021 18:23:04 +0200 [thread overview]
Message-ID: <CAMj1kXGg_oQQ5qo5Tkq1K=62uWL-1S54VHeQ1BzXTWn81AhfSw@mail.gmail.com> (raw)
In-Reply-To: <20210911073232.GS1935@kadam>
On Sat, 11 Sept 2021 at 09:32, Dan Carpenter <dan.carpenter@oracle.com> 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/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.
> >
>
> 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
next prev parent reply other threads:[~2021-09-11 16:23 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-08 12:54 [bug report] crypto: aesni - xts_crypt() return if walk.nbytes is 0 Dan Carpenter
2021-09-10 15:54 ` Shreyansh Chouhan
2021-09-10 15:57 ` Shreyansh Chouhan
2021-09-11 7:32 ` Dan Carpenter
2021-09-11 16:23 ` Ard Biesheuvel [this message]
2021-09-12 5:02 ` Shreyansh Chouhan
2021-09-12 6:45 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMj1kXGg_oQQ5qo5Tkq1K=62uWL-1S54VHeQ1BzXTWn81AhfSw@mail.gmail.com' \
--to=ardb@kernel.org \
--cc=chouhan.shreyansh630@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-crypto@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.