Subject: Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas
 are aligned
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ben Hutchings <ben@decadent.org.uk>
References: <20160410183456.398741366@linuxfoundation.org>
 <20160410183500.908546902@linuxfoundation.org>
 <1460426212.25201.98.camel@decadent.org.uk> <20160412142841.GH7996@kroah.com>
CC: <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>
From: Tom Lendacky <thomas.lendacky@amd.com>
Message-ID: <570D29DD.5000001@amd.com>
Date: Tue, 12 Apr 2016 12:01:17 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160412142841.GH7996@kroah.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2016 17:02:04.8086 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR1201MB1111
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2587
Lines: 79

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
>