xts_crypt() code doesn't call kernel_fpu_end() after calling
kernel_fpu_begin() if walk.nbytes is 0. Add a call to kernel_fpu_end()
for this case.
Reported-by: [email protected]
Signed-off-by: Shreyansh Chouhan <[email protected]>
---
arch/x86/crypto/aesni-intel_glue.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 2144e54a6c89..bd55a0cd7bde 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -894,6 +894,9 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
kernel_fpu_begin();
}
+ if (walk.nbytes == 0)
+ kernel_fpu_end();
+
if (unlikely(tail > 0 && !err)) {
struct scatterlist sg_src[2], sg_dst[2];
struct scatterlist *src, *dst;
--
2.31.1
On Fri, Jul 30, 2021 at 07:11:55PM +0530, Shreyansh Chouhan wrote:
> xts_crypt() code doesn't call kernel_fpu_end() after calling
> kernel_fpu_begin() if walk.nbytes is 0. Add a call to kernel_fpu_end()
> for this case.
>
> Reported-by: [email protected]
> Signed-off-by: Shreyansh Chouhan <[email protected]>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 3 +++
> 1 file changed, 3 insertions(+)
Ard?
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index 2144e54a6c89..bd55a0cd7bde 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -894,6 +894,9 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
> kernel_fpu_begin();
> }
>
> + if (walk.nbytes == 0)
> + kernel_fpu_end();
> +
> if (unlikely(tail > 0 && !err)) {
> struct scatterlist sg_src[2], sg_dst[2];
> struct scatterlist *src, *dst;
> --
> 2.31.1
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, 6 Aug 2021 at 10:23, Herbert Xu <[email protected]> wrote:
>
> On Fri, Jul 30, 2021 at 07:11:55PM +0530, Shreyansh Chouhan wrote:
> > xts_crypt() code doesn't call kernel_fpu_end() after calling
> > kernel_fpu_begin() if walk.nbytes is 0. Add a call to kernel_fpu_end()
> > for this case.
> >
> > Reported-by: [email protected]
> > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > ---
> > arch/x86/crypto/aesni-intel_glue.c | 3 +++
> > 1 file changed, 3 insertions(+)
>
> Ard?
>
> > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > index 2144e54a6c89..bd55a0cd7bde 100644
> > --- a/arch/x86/crypto/aesni-intel_glue.c
> > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > @@ -894,6 +894,9 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
> > kernel_fpu_begin();
> > }
> >
> > + if (walk.nbytes == 0)
> > + kernel_fpu_end();
> > +
Don't we end up calling kernel_fpu_end() twice this way if we do enter
the while() loop at least once?
> > if (unlikely(tail > 0 && !err)) {
> > struct scatterlist sg_src[2], sg_dst[2];
> > struct scatterlist *src, *dst;
> > --
> > 2.31.1
>
> --
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, 6 Aug 2021 at 11:05, Ard Biesheuvel <[email protected]> wrote:
>
> On Fri, 6 Aug 2021 at 10:23, Herbert Xu <[email protected]> wrote:
> >
> > On Fri, Jul 30, 2021 at 07:11:55PM +0530, Shreyansh Chouhan wrote:
> > > xts_crypt() code doesn't call kernel_fpu_end() after calling
> > > kernel_fpu_begin() if walk.nbytes is 0. Add a call to kernel_fpu_end()
> > > for this case.
> > >
> > > Reported-by: [email protected]
> > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > ---
> > > arch/x86/crypto/aesni-intel_glue.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> >
> > Ard?
> >
> > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > > index 2144e54a6c89..bd55a0cd7bde 100644
> > > --- a/arch/x86/crypto/aesni-intel_glue.c
> > > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > > @@ -894,6 +894,9 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
> > > kernel_fpu_begin();
> > > }
> > >
> > > + if (walk.nbytes == 0)
> > > + kernel_fpu_end();
> > > +
>
> Don't we end up calling kernel_fpu_end() twice this way if we do enter
> the while() loop at least once?
>
How about the below instead, does that work?
--- 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)) {
Hi,
On Fri, Aug 06, 2021 at 11:07:43AM +0200, Ard Biesheuvel wrote:
> On Fri, 6 Aug 2021 at 11:05, Ard Biesheuvel <[email protected]> wrote:
> >
> > On Fri, 6 Aug 2021 at 10:23, Herbert Xu <[email protected]> wrote:
> > >
> > > On Fri, Jul 30, 2021 at 07:11:55PM +0530, Shreyansh Chouhan wrote:
> > > > xts_crypt() code doesn't call kernel_fpu_end() after calling
> > > > kernel_fpu_begin() if walk.nbytes is 0. Add a call to kernel_fpu_end()
> > > > for this case.
> > > >
> > > > Reported-by: [email protected]
> > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > ---
> > > > arch/x86/crypto/aesni-intel_glue.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > >
> > > Ard?
> > >
> > > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > > > index 2144e54a6c89..bd55a0cd7bde 100644
> > > > --- a/arch/x86/crypto/aesni-intel_glue.c
> > > > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > > > @@ -894,6 +894,9 @@ static int xts_crypt(struct skcipher_request *req, bool encrypt)
> > > > kernel_fpu_begin();
> > > > }
> > > >
> > > > + if (walk.nbytes == 0)
> > > > + kernel_fpu_end();
> > > > +
> >
> > Don't we end up calling kernel_fpu_end() twice this way if we do enter
> > the while() loop at least once?
> >
Oh ha, we do. I missed that.
>
> How about the below instead, does that work?
>
This should work. I will resend the updated patch.
> --- 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)) {
Thanks a lot for the review.
Regards,
Shreyansh Chouhan
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: [email protected]
Signed-off-by: Shreyansh Chouhan <[email protected]>
---
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, 9 Aug 2021 at 16:10, Shreyansh Chouhan
<[email protected]> 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: [email protected]
> Signed-off-by: Shreyansh Chouhan <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
> ---
> 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: [email protected]
> Signed-off-by: Shreyansh Chouhan <[email protected]>
> ---
> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, 20 Aug 2021 at 10:31, Herbert Xu <[email protected]> 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: [email protected]
> > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > ---
> > 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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: [email protected]
Signed-off-by: Shreyansh Chouhan <[email protected]>
---
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..0fc961bef299 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 (!walk.nbytes)
return err;
if (unlikely(tail > 0 && walk.nbytes < walk.total)) {
--
2.31.1
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 <[email protected]> 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.
On Sun, Aug 22, 2021 at 09:15:14AM +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: [email protected]
> Signed-off-by: Shreyansh Chouhan <[email protected]>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt