Messages in this thread | | | Date | Tue, 12 Apr 2016 07:28:41 -0700 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 4.5 079/238] crypto: ccp - Dont assume export/import areas are aligned |
| |
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?
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
| |