From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-bl2on0076.outbound.protection.outlook.com ([65.55.169.76]:50161 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932900AbcDLRCK (ORCPT ); Tue, 12 Apr 2016 13:02:10 -0400 Subject: Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned To: Greg Kroah-Hartman , Ben Hutchings References: <20160410183456.398741366@linuxfoundation.org> <20160410183500.908546902@linuxfoundation.org> <1460426212.25201.98.camel@decadent.org.uk> <20160412142841.GH7996@kroah.com> CC: , , Herbert Xu From: Tom Lendacky Message-ID: <570D29DD.5000001@amd.com> Date: Tue, 12 Apr 2016 12:01:17 -0500 MIME-Version: 1.0 In-Reply-To: <20160412142841.GH7996@kroah.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 >