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
>
