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;
MIME-Version: 1.0
References: <20210908125440.GA6147@kili> <YTt/uJjgy4jTr+GL@fedora>
 <YTuAerH3S78Xf1Jc@fedora> <20210911073232.GS1935@kadam> <CAMj1kXGg_oQQ5qo5Tkq1K=62uWL-1S54VHeQ1BzXTWn81AhfSw@mail.gmail.com>
 <YT2J8BOAXYY2lOIT@fedora>
In-Reply-To: <YT2J8BOAXYY2lOIT@fedora>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Sun, 12 Sep 2021 08:45:24 +0200
Message-ID: <CAMj1kXHfXHc5K0sjt7_HM0=jAb9sHPqN_36KyfE6uDCnG6Sa9Q@mail.gmail.com>
Subject: Re: [bug report] crypto: aesni - xts_crypt() return if walk.nbytes is 0
To:     Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>,
        Herbert Xu <herbert@gondor.apana.org.au>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, 12 Sept 2021 at 07:02, Shreyansh Chouhan
<chouhan.shreyansh630@gmail.com> wrote:
>
> On Sat, Sep 11, 2021 at 06:23:04PM +0200, Ard Biesheuvel wrote:
> > 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
>
> 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.