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

[Tom Lendacky]

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
>