| Message ID | 20210809141027.860850-1-chouhan.shreyansh630@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: xts_crypt() return if walk.nbytes is 0 | expand |
On Mon, 9 Aug 2021 at 16:10, Shreyansh Chouhan <chouhan.shreyansh630@gmail.com> wrote: > > xts_crypt() code doesn't call kernel_fpu_end() after calling > kernel_fpu_begin() if walk.nbytes is 0. The correct behavior should be > not calling kernel_fpu_begin() if walk.nbytes is 0. > > Reported-by: syzbot+20191dc583eff8602d2d@syzkaller.appspotmail.com > Signed-off-by: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com> Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/crypto/aesni-intel_glue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c > index 388643ca2177..ec6eac57c493 100644 > --- a/arch/x86/crypto/aesni-intel_glue.c > +++ b/arch/x86/crypto/aesni-intel_glue.c > @@ -849,7 +849,7 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt) > return -EINVAL; > > err = skcipher_walk_virt(&walk, req, false); > - if (err) > + if (err || !walk.nbytes) > return err; > > if (unlikely(tail > 0 && walk.nbytes < walk.total)) { > -- > 2.31.1 >
On Mon, Aug 09, 2021 at 07:40:27PM +0530, Shreyansh Chouhan wrote: > xts_crypt() code doesn't call kernel_fpu_end() after calling > kernel_fpu_begin() if walk.nbytes is 0. The correct behavior should be > not calling kernel_fpu_begin() if walk.nbytes is 0. > > Reported-by: syzbot+20191dc583eff8602d2d@syzkaller.appspotmail.com > Signed-off-by: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com> > --- > arch/x86/crypto/aesni-intel_glue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c > index 388643ca2177..ec6eac57c493 100644 > --- a/arch/x86/crypto/aesni-intel_glue.c > +++ b/arch/x86/crypto/aesni-intel_glue.c > @@ -849,7 +849,7 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt) > return -EINVAL; > > err = skcipher_walk_virt(&walk, req, false); > - if (err) > + if (err || !walk.nbytes) > return err; The err check is now redundant because when there is an error nbytes is always zero. Cheers,
On Fri, 20 Aug 2021 at 10:31, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Mon, Aug 09, 2021 at 07:40:27PM +0530, Shreyansh Chouhan wrote: > > xts_crypt() code doesn't call kernel_fpu_end() after calling > > kernel_fpu_begin() if walk.nbytes is 0. The correct behavior should be > > not calling kernel_fpu_begin() if walk.nbytes is 0. > > > > Reported-by: syzbot+20191dc583eff8602d2d@syzkaller.appspotmail.com > > Signed-off-by: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com> > > --- > > arch/x86/crypto/aesni-intel_glue.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c > > index 388643ca2177..ec6eac57c493 100644 > > --- a/arch/x86/crypto/aesni-intel_glue.c > > +++ b/arch/x86/crypto/aesni-intel_glue.c > > @@ -849,7 +849,7 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt) > > return -EINVAL; > > > > err = skcipher_walk_virt(&walk, req, false); > > - if (err) > > + if (err || !walk.nbytes) > > return err; > > The err check is now redundant because when there is an error > nbytes is always zero. > In spite of that, I have a slight preference for this version, given that it makes it obvious that we bail on two separate conditions: - an error has occurred - no error has occurred but the resulting walk is empty Testing walk.nbytes only needlessly obfuscates the code, as we need to return 'err' in the end anyway.
On Fri, Aug 20, 2021 at 01:14:52PM +0200, Ard Biesheuvel wrote: > > In spite of that, I have a slight preference for this version, given > that it makes it obvious that we bail on two separate conditions: > - an error has occurred > - no error has occurred but the resulting walk is empty > > Testing walk.nbytes only needlessly obfuscates the code, as we need to > return 'err' in the end anyway. I disagree, this is how most skcipher walkers are structured, they never explicitly test on err and only terminate the loop when walk->nbytes hits zero, in which case err is returned as is. I don't see why this particular skcipher walker should deviate from that paradigm. In fact it is exactly that deviation that caused the bug in the first instance. Cheers,
Hi, Thank you Ard and Herebrt for your reviews. I have sent an updated patch. Sorry for the delay. Regards, Shreyansh
On Fri, 20 Aug 2021 at 14:53, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Fri, Aug 20, 2021 at 01:14:52PM +0200, Ard Biesheuvel wrote: > > > > In spite of that, I have a slight preference for this version, given > > that it makes it obvious that we bail on two separate conditions: > > - an error has occurred > > - no error has occurred but the resulting walk is empty > > > > Testing walk.nbytes only needlessly obfuscates the code, as we need to > > return 'err' in the end anyway. > > I disagree, this is how most skcipher walkers are structured, they > never explicitly test on err and only terminate the loop when > walk->nbytes hits zero, in which case err is returned as is. > > I don't see why this particular skcipher walker should deviate > from that paradigm. In fact it is exactly that deviation that > caused the bug in the first instance. > Fair enough.
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c index 388643ca2177..ec6eac57c493 100644 --- a/arch/x86/crypto/aesni-intel_glue.c +++ b/arch/x86/crypto/aesni-intel_glue.c @@ -849,7 +849,7 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt) return -EINVAL; err = skcipher_walk_virt(&walk, req, false); - if (err) + if (err || !walk.nbytes) return err; if (unlikely(tail > 0 && walk.nbytes < walk.total)) {
xts_crypt() code doesn't call kernel_fpu_end() after calling kernel_fpu_begin() if walk.nbytes is 0. The correct behavior should be not calling kernel_fpu_begin() if walk.nbytes is 0. Reported-by: syzbot+20191dc583eff8602d2d@syzkaller.appspotmail.com Signed-off-by: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com> --- arch/x86/crypto/aesni-intel_glue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)