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 > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html