Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1374856 > unrolled thread

[PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-04-10 20:50 +0200
    Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import  areas are aligned Ben Hutchings <ben@decadent.org.uk> - 2016-04-12 04:00 +0200
      Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import  areas are aligned Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-04-12 16:30 +0200
        Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas  are aligned Tom Lendacky <thomas.lendacky@amd.com> - 2016-04-12 19:10 +0200
          Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import  areas are aligned Ben Hutchings <ben@decadent.org.uk> - 2016-04-12 19:30 +0200

#1374856 — [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

4.5-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Tom Lendacky <thomas.lendacky@amd.com>

commit b31dde2a5cb1bf764282abf934266b7193c2bc7c upstream.

Use a local variable for the exported and imported state so that
alignment is not an issue. On export, set a local variable from the
request context and then memcpy the contents of the local variable to
the export memory area. On import, memcpy the import memory area into
a local variable and then use the local variable to set the request
context.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 drivers/crypto/ccp/ccp-crypto-aes-cmac.c |   26 ++++++++++++++---------
 drivers/crypto/ccp/ccp-crypto-sha.c      |   34 ++++++++++++++++++-------------
 2 files changed, 36 insertions(+), 24 deletions(-)

--- a/drivers/crypto/ccp/ccp-crypto-aes-cmac.c
+++ b/drivers/crypto/ccp/ccp-crypto-aes-cmac.c
@@ -223,12 +223,15 @@ static int ccp_aes_cmac_digest(struct ah
 static int ccp_aes_cmac_export(struct ahash_request *req, void *out)
 {
 	struct ccp_aes_cmac_req_ctx *rctx = ahash_request_ctx(req);
-	struct ccp_aes_cmac_exp_ctx *state = out;
+	struct ccp_aes_cmac_exp_ctx state;
 
-	state->null_msg = rctx->null_msg;
-	memcpy(state->iv, rctx->iv, sizeof(state->iv));
-	state->buf_count = rctx->buf_count;
-	memcpy(state->buf, rctx->buf, sizeof(state->buf));
+	state.null_msg = rctx->null_msg;
+	memcpy(state.iv, rctx->iv, sizeof(state.iv));
+	state.buf_count = rctx->buf_count;
+	memcpy(state.buf, rctx->buf, sizeof(state.buf));
+
+	/* 'out' may not be aligned so memcpy from local variable */
+	memcpy(out, &state, sizeof(state));
 
 	return 0;
 }
@@ -236,12 +239,15 @@ static int ccp_aes_cmac_export(struct ah
 static int ccp_aes_cmac_import(struct ahash_request *req, const void *in)
 {
 	struct ccp_aes_cmac_req_ctx *rctx = ahash_request_ctx(req);
-	const struct ccp_aes_cmac_exp_ctx *state = in;
+	struct ccp_aes_cmac_exp_ctx state;
+
+	/* 'in' may not be aligned so memcpy to local variable */
+	memcpy(&state, in, sizeof(state));
 
-	rctx->null_msg = state->null_msg;
-	memcpy(rctx->iv, state->iv, sizeof(rctx->iv));
-	rctx->buf_count = state->buf_count;
-	memcpy(rctx->buf, state->buf, sizeof(rctx->buf));
+	rctx->null_msg = state.null_msg;
+	memcpy(rctx->iv, state.iv, sizeof(rctx->iv));
+	rctx->buf_count = state.buf_count;
+	memcpy(rctx->buf, state.buf, sizeof(rctx->buf));
 
 	return 0;
 }
--- a/drivers/crypto/ccp/ccp-crypto-sha.c
+++ b/drivers/crypto/ccp/ccp-crypto-sha.c
@@ -210,14 +210,17 @@ static int ccp_sha_digest(struct ahash_r
 static int ccp_sha_export(struct ahash_request *req, void *out)
 {
 	struct ccp_sha_req_ctx *rctx = ahash_request_ctx(req);
-	struct ccp_sha_exp_ctx *state = out;
+	struct ccp_sha_exp_ctx state;
 
-	state->type = rctx->type;
-	state->msg_bits = rctx->msg_bits;
-	state->first = rctx->first;
-	memcpy(state->ctx, rctx->ctx, sizeof(state->ctx));
-	state->buf_count = rctx->buf_count;
-	memcpy(state->buf, rctx->buf, sizeof(state->buf));
+	state.type = rctx->type;
+	state.msg_bits = rctx->msg_bits;
+	state.first = rctx->first;
+	memcpy(state.ctx, rctx->ctx, sizeof(state.ctx));
+	state.buf_count = rctx->buf_count;
+	memcpy(state.buf, rctx->buf, sizeof(state.buf));
+
+	/* 'out' may not be aligned so memcpy from local variable */
+	memcpy(out, &state, sizeof(state));
 
 	return 0;
 }
@@ -225,14 +228,17 @@ static int ccp_sha_export(struct ahash_r
 static int ccp_sha_import(struct ahash_request *req, const void *in)
 {
 	struct ccp_sha_req_ctx *rctx = ahash_request_ctx(req);
-	const struct ccp_sha_exp_ctx *state = in;
+	struct ccp_sha_exp_ctx state;
+
+	/* 'in' may not be aligned so memcpy to local variable */
+	memcpy(&state, in, sizeof(state));
 
-	rctx->type = state->type;
-	rctx->msg_bits = state->msg_bits;
-	rctx->first = state->first;
-	memcpy(rctx->ctx, state->ctx, sizeof(rctx->ctx));
-	rctx->buf_count = state->buf_count;
-	memcpy(rctx->buf, state->buf, sizeof(rctx->buf));
+	rctx->type = state.type;
+	rctx->msg_bits = state.msg_bits;
+	rctx->first = state.first;
+	memcpy(rctx->ctx, state.ctx, sizeof(rctx->ctx));
+	rctx->buf_count = state.buf_count;
+	memcpy(rctx->buf, state.buf, sizeof(rctx->buf));
 
 	return 0;
 }

[toc] | [next] | [standalone]

#1376403 — Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

[Multipart message — attachments visible in raw view] — view raw

On Sun, 2016-04-10 at 11:34 -0700, Greg Kroah-Hartman wrote:
> 4.5-stable review patch.  If anyone has any objections, please let me know.

I object, because this introduces an information leak.

[...]
> --- a/drivers/crypto/ccp/ccp-crypto-sha.c
> +++ b/drivers/crypto/ccp/ccp-crypto-sha.c
> @@ -210,14 +210,17 @@ static int ccp_sha_digest(struct ahash_r
>  static int ccp_sha_export(struct ahash_request *req, void *out)
>  {
>  	struct ccp_sha_req_ctx *rctx = ahash_request_ctx(req);
> -	struct ccp_sha_exp_ctx *state = out;
> +	struct ccp_sha_exp_ctx state;

The structure was defined in the previous patch as:

> +struct ccp_sha_exp_ctx {
> +	enum ccp_sha_type type;

There will be padding between type and msg_bits on most architectures.

> +	u64 msg_bits;
> +	unsigned int first;
> +
> +	u8 ctx[MAX_SHA_CONTEXT_SIZE];
> +
> +	unsigned int buf_count;
> +	u8 buf[MAX_SHA_BLOCK_SIZE];

And more padding at the end of the structure.

> +};

Back to the code:

> -	state->type = rctx->type;
> -	state->msg_bits = rctx->msg_bits;
> -	state->first = rctx->first;
> -	memcpy(state->ctx, rctx->ctx, sizeof(state->ctx));
> -	state->buf_count = rctx->buf_count;
> -	memcpy(state->buf, rctx->buf, sizeof(state->buf));
> +	state.type = rctx->type;
> +	state.msg_bits = rctx->msg_bits;
> +	state.first = rctx->first;
> +	memcpy(state.ctx, rctx->ctx, sizeof(state.ctx));
> +	state.buf_count = rctx->buf_count;
> +	memcpy(state.buf, rctx->buf, sizeof(state.buf));
> +
> +	/* 'out' may not be aligned so memcpy from local variable */
> +	memcpy(out, &state, sizeof(state));
[...]

The padding was not initialised, but here we copy it to userland.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

[toc] | [prev] | [next] | [standalone]

#1376851 — Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

On Tue, Apr 12, 2016 at 02:56:52AM +0100, Ben Hutchings wrote:
> On Sun, 2016-04-10 at 11:34 -0700, Greg Kroah-Hartman wrote:
> > 4.5-stable review patch.  If anyone has any objections, please let me know.
> 
> I object, because this introduces an information leak.
> 
> [...]
> > --- a/drivers/crypto/ccp/ccp-crypto-sha.c
> > +++ b/drivers/crypto/ccp/ccp-crypto-sha.c
> > @@ -210,14 +210,17 @@ static int ccp_sha_digest(struct ahash_r
> >  static int ccp_sha_export(struct ahash_request *req, void *out)
> >  {
> >  	struct ccp_sha_req_ctx *rctx = ahash_request_ctx(req);
> > -	struct ccp_sha_exp_ctx *state = out;
> > +	struct ccp_sha_exp_ctx state;
> 
> The structure was defined in the previous patch as:
> 
> > +struct ccp_sha_exp_ctx {
> > +	enum ccp_sha_type type;
> 
> There will be padding between type and msg_bits on most architectures.
> 
> > +	u64 msg_bits;
> > +	unsigned int first;
> > +
> > +	u8 ctx[MAX_SHA_CONTEXT_SIZE];
> > +
> > +	unsigned int buf_count;
> > +	u8 buf[MAX_SHA_BLOCK_SIZE];
> 
> And more padding at the end of the structure.
> 
> > +};
> 
> Back to the code:
> 
> > -	state->type = rctx->type;
> > -	state->msg_bits = rctx->msg_bits;
> > -	state->first = rctx->first;
> > -	memcpy(state->ctx, rctx->ctx, sizeof(state->ctx));
> > -	state->buf_count = rctx->buf_count;
> > -	memcpy(state->buf, rctx->buf, sizeof(state->buf));
> > +	state.type = rctx->type;
> > +	state.msg_bits = rctx->msg_bits;
> > +	state.first = rctx->first;
> > +	memcpy(state.ctx, rctx->ctx, sizeof(state.ctx));
> > +	state.buf_count = rctx->buf_count;
> > +	memcpy(state.buf, rctx->buf, sizeof(state.buf));
> > +
> > +	/* 'out' may not be aligned so memcpy from local variable */
> > +	memcpy(out, &state, sizeof(state));
> [...]
> 
> The padding was not initialised, but here we copy it to userland.

Nice catch.  Given that the user/kernel structure here doesn't seem very
sane (implicit padding, etc.), shouldn't that be where this is fixed up
to be a properly packed structure?  Or have padding where needed, along
with a memset() call?

I'll leave this here, but will be expecting a follow-on patch to fix up
the issues from the crypto developers.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1377028 — Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

On 04/12/2016 09:28 AM, Greg Kroah-Hartman wrote:
> On Tue, Apr 12, 2016 at 02:56:52AM +0100, Ben Hutchings wrote:
>> On Sun, 2016-04-10 at 11:34 -0700, Greg Kroah-Hartman wrote:
>>> 4.5-stable review patch.  If anyone has any objections, please let me know.
>>
>> I object, because this introduces an information leak.
>>
>> [...]
>>> --- a/drivers/crypto/ccp/ccp-crypto-sha.c
>>> +++ b/drivers/crypto/ccp/ccp-crypto-sha.c
>>> @@ -210,14 +210,17 @@ static int ccp_sha_digest(struct ahash_r
>>>  static int ccp_sha_export(struct ahash_request *req, void *out)
>>>  {
>>>  	struct ccp_sha_req_ctx *rctx = ahash_request_ctx(req);
>>> -	struct ccp_sha_exp_ctx *state = out;
>>> +	struct ccp_sha_exp_ctx state;
>>
>> The structure was defined in the previous patch as:
>>
>>> +struct ccp_sha_exp_ctx {
>>> +	enum ccp_sha_type type;
>>
>> There will be padding between type and msg_bits on most architectures.
>>
>>> +	u64 msg_bits;
>>> +	unsigned int first;
>>> +
>>> +	u8 ctx[MAX_SHA_CONTEXT_SIZE];
>>> +
>>> +	unsigned int buf_count;
>>> +	u8 buf[MAX_SHA_BLOCK_SIZE];
>>
>> And more padding at the end of the structure.
>>
>>> +};
>>
>> Back to the code:
>>
>>> -	state->type = rctx->type;
>>> -	state->msg_bits = rctx->msg_bits;
>>> -	state->first = rctx->first;
>>> -	memcpy(state->ctx, rctx->ctx, sizeof(state->ctx));
>>> -	state->buf_count = rctx->buf_count;
>>> -	memcpy(state->buf, rctx->buf, sizeof(state->buf));
>>> +	state.type = rctx->type;
>>> +	state.msg_bits = rctx->msg_bits;
>>> +	state.first = rctx->first;
>>> +	memcpy(state.ctx, rctx->ctx, sizeof(state.ctx));
>>> +	state.buf_count = rctx->buf_count;
>>> +	memcpy(state.buf, rctx->buf, sizeof(state.buf));
>>> +
>>> +	/* 'out' may not be aligned so memcpy from local variable */
>>> +	memcpy(out, &state, sizeof(state));
>> [...]
>>
>> The padding was not initialised, but here we copy it to userland.
> 
> Nice catch.  Given that the user/kernel structure here doesn't seem very
> sane (implicit padding, etc.), shouldn't that be where this is fixed up
> to be a properly packed structure?  Or have padding where needed, along
> with a memset() call?

The structure is not meant for use outside the kernel - it's an opaque
blob that will be processed by the driver import function. So would it
be enough to just memset the struct ccp_sha_exp_ctx state variable to 0
before setting and copying it?  That should take care of any padding not
being initialized.

Thanks,
Tom

> 
> I'll leave this here, but will be expecting a follow-on patch to fix up
> the issues from the crypto developers.
> 
> thanks,
> 
> greg k-h
> 

[toc] | [prev] | [next] | [standalone]

#1377042 — Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-04-12 at 12:01 -0500, Tom Lendacky wrote:
> On 04/12/2016 09:28 AM, Greg Kroah-Hartman wrote:
> > 
> > On Tue, Apr 12, 2016 at 02:56:52AM +0100, Ben Hutchings wrote:
> > > 
> > > On Sun, 2016-04-10 at 11:34 -0700, Greg Kroah-Hartman wrote:
[...]
> > > > -	state->type = rctx->type;
> > > > -	state->msg_bits = rctx->msg_bits;
> > > > -	state->first = rctx->first;
> > > > -	memcpy(state->ctx, rctx->ctx, sizeof(state->ctx));
> > > > -	state->buf_count = rctx->buf_count;
> > > > -	memcpy(state->buf, rctx->buf, sizeof(state->buf));
> > > > +	state.type = rctx->type;
> > > > +	state.msg_bits = rctx->msg_bits;
> > > > +	state.first = rctx->first;
> > > > +	memcpy(state.ctx, rctx->ctx, sizeof(state.ctx));
> > > > +	state.buf_count = rctx->buf_count;
> > > > +	memcpy(state.buf, rctx->buf, sizeof(state.buf));
> > > > +
> > > > +	/* 'out' may not be aligned so memcpy from local variable */
> > > > +	memcpy(out, &state, sizeof(state));
> > > [...]
> > > 
> > > The padding was not initialised, but here we copy it to userland.
> > Nice catch.  Given that the user/kernel structure here doesn't seem very
> > sane (implicit padding, etc.), shouldn't that be where this is fixed up
> > to be a properly packed structure?  Or have padding where needed, along
> > with a memset() call?
> The structure is not meant for use outside the kernel - it's an opaque
> blob that will be processed by the driver import function. So would it
> be enough to just memset the struct ccp_sha_exp_ctx state variable to 0
> before setting and copying it?  That should take care of any padding not
> being initialized.

I think that would be enough.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web