linux-kernel - Re: [PATCH V7 5/7] crypto: AES CBC multi-buffer glue code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 09 Jan 2018 13:07:24 -0800
From:   Megha Dey <megha.dey@...el.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     tim.c.chen@...ux.intel.com, davem@...emloft.net,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V7 5/7] crypto: AES CBC multi-buffer glue code

On Thu, 2017-08-03 at 13:27 +0800, Herbert Xu wrote:
> On Tue, Jul 25, 2017 at 07:09:58PM -0700, Megha Dey wrote:
> >
> > +/* notify the caller of progress ; request still stays in queue */
> > +
> > +static void notify_callback(struct mcryptd_skcipher_request_ctx *rctx,
> > +			    struct mcryptd_alg_cstate *cstate,
> > +			    int err)
> > +{
> > +	struct skcipher_request *req = cast_mcryptd_ctx_to_req(rctx);
> > +
> > +	local_bh_disable();
> > +	rctx->complete(&req->base, err);
> > +	local_bh_enable();
> > +}
> 
> Please explain why you have this crazy construct that does async
> operations behind the crypto API's back while pretending to be sync
> by always returning zero?
> 
> Why is this even needed now that you have switched the underlying
> implementation to be async?

Hi Herbert,

In the next version, I have removed this construct. After giving it some
thought, I realise this is actually incorrect. Hopefully now, both the
outer and inner algorithm are async. 
> 
> > +	/* from mcryptd, we need to callback */
> > +	if (irqs_disabled())
> > +		rctx->complete(&req->base, err);
> > +	else {
> > +		local_bh_disable();
> > +		rctx->complete(&req->base, err);
> > +		local_bh_enable();
> > +	}
> 
> I complained about this the first time around and yet this crap is
> still there.

Sorry about that, will get rid of the context check in the next version.
> 
> Cheers,