diff mbox series

[RFC/RFT,09/18] crypto: streebog - fix unaligned memory accesses

Message ID 20190331200428.26597-10-ebiggers@kernel.org (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series crypto: fuzz algorithms against their generic implementation | expand

Commit Message

Eric Biggers March 31, 2019, 8:04 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Don't cast the data buffer directly to streebog_uint512, as this
violates alignment rules.

Fixes: fe18957e8e87 ("crypto: streebog - add Streebog hash function")
Cc: Vitaly Chikunov <vt@altlinux.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/streebog_generic.c | 25 +++++++++++++------------
 include/crypto/streebog.h |  5 ++++-
 2 files changed, 17 insertions(+), 13 deletions(-)

Comments

Vitaly Chikunov March 31, 2019, 9:47 p.m. UTC | #1 
Eric,

On Sun, Mar 31, 2019 at 01:04:19PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Don't cast the data buffer directly to streebog_uint512, as this
> violates alignment rules.
> 
> Fixes: fe18957e8e87 ("crypto: streebog - add Streebog hash function")
> Cc: Vitaly Chikunov <vt@altlinux.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/streebog_generic.c | 25 +++++++++++++------------
>  include/crypto/streebog.h |  5 ++++-
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/crypto/streebog_generic.c b/crypto/streebog_generic.c
> index 5a2eafed9c29f..b82fc3d79aa15 100644
> --- a/crypto/streebog_generic.c
> +++ b/crypto/streebog_generic.c
> @@ -996,7 +996,7 @@ static void streebog_add512(const struct streebog_uint512 *x,
>  
>  static void streebog_g(struct streebog_uint512 *h,
>  		       const struct streebog_uint512 *N,
> -		       const u8 *m)
> +		       const struct streebog_uint512 *m)
>  {
>  	struct streebog_uint512 Ki, data;
>  	unsigned int i;
> @@ -1005,7 +1005,7 @@ static void streebog_g(struct streebog_uint512 *h,
>  
>  	/* Starting E() */
>  	Ki = data;
> -	streebog_xlps(&Ki, (const struct streebog_uint512 *)&m[0], &data);
> +	streebog_xlps(&Ki, m, &data);
>  
>  	for (i = 0; i < 11; i++)
>  		streebog_round(i, &Ki, &data);
> @@ -1015,16 +1015,19 @@ static void streebog_g(struct streebog_uint512 *h,
>  	/* E() done */
>  
>  	streebog_xor(&data, h, &data);
> -	streebog_xor(&data, (const struct streebog_uint512 *)&m[0], h);
> +	streebog_xor(&data, m, h);
>  }
>  
>  static void streebog_stage2(struct streebog_state *ctx, const u8 *data)
>  {
> -	streebog_g(&ctx->h, &ctx->N, data);
> +	struct streebog_uint512 m;
> +
> +	memcpy(&m, data, sizeof(m));
> +
> +	streebog_g(&ctx->h, &ctx->N, &m);
>  
>  	streebog_add512(&ctx->N, &buffer512, &ctx->N);
> -	streebog_add512(&ctx->Sigma, (const struct streebog_uint512 *)data,
> -			&ctx->Sigma);
> +	streebog_add512(&ctx->Sigma, &m, &ctx->Sigma);
>  }

As I understand, this is the actual fix.

Reviewed-by: Vitaly Chikunov <vt@altlinux.org>

Thanks much!

>  
>  static void streebog_stage3(struct streebog_state *ctx)
> @@ -1034,13 +1037,11 @@ static void streebog_stage3(struct streebog_state *ctx)
>  	buf.qword[0] = cpu_to_le64(ctx->fillsize << 3);
>  	streebog_pad(ctx);
>  
> -	streebog_g(&ctx->h, &ctx->N, (const u8 *)&ctx->buffer);
> +	streebog_g(&ctx->h, &ctx->N, &ctx->m);
>  	streebog_add512(&ctx->N, &buf, &ctx->N);
> -	streebog_add512(&ctx->Sigma,
> -			(const struct streebog_uint512 *)&ctx->buffer[0],
> -			&ctx->Sigma);
> -	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->N);
> -	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->Sigma);
> +	streebog_add512(&ctx->Sigma, &ctx->m, &ctx->Sigma);
> +	streebog_g(&ctx->h, &buffer0, &ctx->N);
> +	streebog_g(&ctx->h, &buffer0, &ctx->Sigma);
>  	memcpy(&ctx->hash, &ctx->h, sizeof(struct streebog_uint512));
>  }
>  
> diff --git a/include/crypto/streebog.h b/include/crypto/streebog.h
> index 856e32af86574..cae1b4a019713 100644
> --- a/include/crypto/streebog.h
> +++ b/include/crypto/streebog.h
> @@ -23,7 +23,10 @@ struct streebog_uint512 {
>  };
>  
>  struct streebog_state {
> -	u8 buffer[STREEBOG_BLOCK_SIZE];
> +	union {
> +		u8 buffer[STREEBOG_BLOCK_SIZE];
> +		struct streebog_uint512 m;
> +	};
>  	struct streebog_uint512 hash;
>  	struct streebog_uint512 h;
>  	struct streebog_uint512 N;
> -- 
> 2.21.0
Vitaly Chikunov April 2, 2019, 4:15 p.m. UTC | #2 
Eric,

On Mon, Apr 01, 2019 at 12:47:19AM +0300, Vitaly Chikunov wrote:
> On Sun, Mar 31, 2019 at 01:04:19PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Don't cast the data buffer directly to streebog_uint512, as this
> > violates alignment rules.
> > 
> > Fixes: fe18957e8e87 ("crypto: streebog - add Streebog hash function")
> > Cc: Vitaly Chikunov <vt@altlinux.org>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  crypto/streebog_generic.c | 25 +++++++++++++------------
> >  include/crypto/streebog.h |  5 ++++-
> >  2 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/crypto/streebog_generic.c b/crypto/streebog_generic.c
> > index 5a2eafed9c29f..b82fc3d79aa15 100644
> > --- a/crypto/streebog_generic.c
> > +++ b/crypto/streebog_generic.c
> > @@ -996,7 +996,7 @@ static void streebog_add512(const struct streebog_uint512 *x,
> >  
> >  static void streebog_g(struct streebog_uint512 *h,
> >  		       const struct streebog_uint512 *N,
> > -		       const u8 *m)
> > +		       const struct streebog_uint512 *m)
> >  {
> >  	struct streebog_uint512 Ki, data;
> >  	unsigned int i;
> > @@ -1005,7 +1005,7 @@ static void streebog_g(struct streebog_uint512 *h,
> >  
> >  	/* Starting E() */
> >  	Ki = data;
> > -	streebog_xlps(&Ki, (const struct streebog_uint512 *)&m[0], &data);
> > +	streebog_xlps(&Ki, m, &data);
> >  
> >  	for (i = 0; i < 11; i++)
> >  		streebog_round(i, &Ki, &data);
> > @@ -1015,16 +1015,19 @@ static void streebog_g(struct streebog_uint512 *h,
> >  	/* E() done */
> >  
> >  	streebog_xor(&data, h, &data);
> > -	streebog_xor(&data, (const struct streebog_uint512 *)&m[0], h);
> > +	streebog_xor(&data, m, h);
> >  }
> >  
> >  static void streebog_stage2(struct streebog_state *ctx, const u8 *data)
> >  {
> > -	streebog_g(&ctx->h, &ctx->N, data);
> > +	struct streebog_uint512 m;
> > +
> > +	memcpy(&m, data, sizeof(m));
> > +
> > +	streebog_g(&ctx->h, &ctx->N, &m);
> >  
> >  	streebog_add512(&ctx->N, &buffer512, &ctx->N);
> > -	streebog_add512(&ctx->Sigma, (const struct streebog_uint512 *)data,
> > -			&ctx->Sigma);
> > +	streebog_add512(&ctx->Sigma, &m, &ctx->Sigma);
> >  }
> 
> As I understand, this is the actual fix.

Probably, even better would be to use CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
to optimize out memcpy() for such architectures.

Thanks,

> Reviewed-by: Vitaly Chikunov <vt@altlinux.org>
> 
> Thanks much!
> 
> >  
> >  static void streebog_stage3(struct streebog_state *ctx)
> > @@ -1034,13 +1037,11 @@ static void streebog_stage3(struct streebog_state *ctx)
> >  	buf.qword[0] = cpu_to_le64(ctx->fillsize << 3);
> >  	streebog_pad(ctx);
> >  
> > -	streebog_g(&ctx->h, &ctx->N, (const u8 *)&ctx->buffer);
> > +	streebog_g(&ctx->h, &ctx->N, &ctx->m);
> >  	streebog_add512(&ctx->N, &buf, &ctx->N);
> > -	streebog_add512(&ctx->Sigma,
> > -			(const struct streebog_uint512 *)&ctx->buffer[0],
> > -			&ctx->Sigma);
> > -	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->N);
> > -	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->Sigma);
> > +	streebog_add512(&ctx->Sigma, &ctx->m, &ctx->Sigma);
> > +	streebog_g(&ctx->h, &buffer0, &ctx->N);
> > +	streebog_g(&ctx->h, &buffer0, &ctx->Sigma);
> >  	memcpy(&ctx->hash, &ctx->h, sizeof(struct streebog_uint512));
> >  }
> >  
> > diff --git a/include/crypto/streebog.h b/include/crypto/streebog.h
> > index 856e32af86574..cae1b4a019713 100644
> > --- a/include/crypto/streebog.h
> > +++ b/include/crypto/streebog.h
> > @@ -23,7 +23,10 @@ struct streebog_uint512 {
> >  };
> >  
> >  struct streebog_state {
> > -	u8 buffer[STREEBOG_BLOCK_SIZE];
> > +	union {
> > +		u8 buffer[STREEBOG_BLOCK_SIZE];
> > +		struct streebog_uint512 m;
> > +	};
> >  	struct streebog_uint512 hash;
> >  	struct streebog_uint512 h;
> >  	struct streebog_uint512 N;
> > -- 
> > 2.21.0
Eric Biggers April 2, 2019, 4:57 p.m. UTC | #3 
On Tue, Apr 02, 2019 at 07:15:57PM +0300, Vitaly Chikunov wrote:
> > >  
> > >  static void streebog_stage2(struct streebog_state *ctx, const u8 *data)
> > >  {
> > > -	streebog_g(&ctx->h, &ctx->N, data);
> > > +	struct streebog_uint512 m;
> > > +
> > > +	memcpy(&m, data, sizeof(m));
> > > +
> > > +	streebog_g(&ctx->h, &ctx->N, &m);
> > >  
> > >  	streebog_add512(&ctx->N, &buffer512, &ctx->N);
> > > -	streebog_add512(&ctx->Sigma, (const struct streebog_uint512 *)data,
> > > -			&ctx->Sigma);
> > > +	streebog_add512(&ctx->Sigma, &m, &ctx->Sigma);
> > >  }
> > 
> > As I understand, this is the actual fix.
> 
> Probably, even better would be to use CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> to optimize out memcpy() for such architectures.
> 

Having multiple code paths is more error-prone, and contrary to popular belief
you can't break alignment rules without informing the compiler, even when
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.  See
https://patchwork.kernel.org/cover/10631429/.

If you want to code up something yourself using get_unaligned_le64() or
__attribute__((packed)), that probably would be the way to go.  But for now I
just want to fix it to not cause a test failure.  I don't have any particular
interest in optimizing Streebog myself, especially the C implementation (if you
really cared about performance you'd add an assembly implementation).

- Eric
diff mbox series

Patch

diff --git a/crypto/streebog_generic.c b/crypto/streebog_generic.c
index 5a2eafed9c29f..b82fc3d79aa15 100644
--- a/crypto/streebog_generic.c
+++ b/crypto/streebog_generic.c
@@ -996,7 +996,7 @@  static void streebog_add512(const struct streebog_uint512 *x,
 
 static void streebog_g(struct streebog_uint512 *h,
 		       const struct streebog_uint512 *N,
-		       const u8 *m)
+		       const struct streebog_uint512 *m)
 {
 	struct streebog_uint512 Ki, data;
 	unsigned int i;
@@ -1005,7 +1005,7 @@  static void streebog_g(struct streebog_uint512 *h,
 
 	/* Starting E() */
 	Ki = data;
-	streebog_xlps(&Ki, (const struct streebog_uint512 *)&m[0], &data);
+	streebog_xlps(&Ki, m, &data);
 
 	for (i = 0; i < 11; i++)
 		streebog_round(i, &Ki, &data);
@@ -1015,16 +1015,19 @@  static void streebog_g(struct streebog_uint512 *h,
 	/* E() done */
 
 	streebog_xor(&data, h, &data);
-	streebog_xor(&data, (const struct streebog_uint512 *)&m[0], h);
+	streebog_xor(&data, m, h);
 }
 
 static void streebog_stage2(struct streebog_state *ctx, const u8 *data)
 {
-	streebog_g(&ctx->h, &ctx->N, data);
+	struct streebog_uint512 m;
+
+	memcpy(&m, data, sizeof(m));
+
+	streebog_g(&ctx->h, &ctx->N, &m);
 
 	streebog_add512(&ctx->N, &buffer512, &ctx->N);
-	streebog_add512(&ctx->Sigma, (const struct streebog_uint512 *)data,
-			&ctx->Sigma);
+	streebog_add512(&ctx->Sigma, &m, &ctx->Sigma);
 }
 
 static void streebog_stage3(struct streebog_state *ctx)
@@ -1034,13 +1037,11 @@  static void streebog_stage3(struct streebog_state *ctx)
 	buf.qword[0] = cpu_to_le64(ctx->fillsize << 3);
 	streebog_pad(ctx);
 
-	streebog_g(&ctx->h, &ctx->N, (const u8 *)&ctx->buffer);
+	streebog_g(&ctx->h, &ctx->N, &ctx->m);
 	streebog_add512(&ctx->N, &buf, &ctx->N);
-	streebog_add512(&ctx->Sigma,
-			(const struct streebog_uint512 *)&ctx->buffer[0],
-			&ctx->Sigma);
-	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->N);
-	streebog_g(&ctx->h, &buffer0, (const u8 *)&ctx->Sigma);
+	streebog_add512(&ctx->Sigma, &ctx->m, &ctx->Sigma);
+	streebog_g(&ctx->h, &buffer0, &ctx->N);
+	streebog_g(&ctx->h, &buffer0, &ctx->Sigma);
 	memcpy(&ctx->hash, &ctx->h, sizeof(struct streebog_uint512));
 }
 
diff --git a/include/crypto/streebog.h b/include/crypto/streebog.h
index 856e32af86574..cae1b4a019713 100644
--- a/include/crypto/streebog.h
+++ b/include/crypto/streebog.h
@@ -23,7 +23,10 @@  struct streebog_uint512 {
 };
 
 struct streebog_state {
-	u8 buffer[STREEBOG_BLOCK_SIZE];
+	union {
+		u8 buffer[STREEBOG_BLOCK_SIZE];
+		struct streebog_uint512 m;
+	};
 	struct streebog_uint512 hash;
 	struct streebog_uint512 h;
 	struct streebog_uint512 N;