linux-kernel - Re: [Patch V5 1/7] crypto: Multi-buffer encryption infrastructure support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1496951574.8042.15.camel@megha-Z97X-UD7-TH>
Date:   Thu, 08 Jun 2017 12:52:54 -0700
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 V5 1/7] crypto: Multi-buffer encryption infrastructure
 support

On Mon, 2017-04-24 at 17:00 +0800, Herbert Xu wrote:
> On Thu, Apr 20, 2017 at 01:50:34PM -0700, Megha Dey wrote:
> >
> > +static int simd_skcipher_decrypt_mb(struct skcipher_request *req)
> > +{
> > +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> > +	struct simd_skcipher_ctx_mb *ctx = crypto_skcipher_ctx(tfm);
> > +	struct skcipher_request *subreq;
> > +	struct crypto_skcipher *child;
> > +
> > +	subreq = skcipher_request_ctx(req);
> > +	*subreq = *req;
> > +
> > +	child = &ctx->mcryptd_tfm->base;
> > +
> > +	skcipher_request_set_tfm(subreq, child);
> > +
> > +	return crypto_skcipher_decrypt(subreq);
> > +}
> 
> Why did you add the code here in simd.c? This doesn't seem to have
> anything to do with kernel SIMD instructions.
> 
> From your later patch I see that what you want is simply a wrapper
> around a synchronous internal algorithm.  That is indeed similar
> in purpose to simd.c, but I still find it weird to be adding this
> code here.
> 
> My suggestion is to instead move this code to mcryptd.c.  It's
> a bit fiddly because mcryptd already exists as a template.  But
> you should still be able to create a seaprate mcryptd interface
> for skcipher in the way that simd does it.  In time we can convert
> mcryptd hash to this model too.
> 
> Also you adopted the simd compat naming scheme.  Please don't do
> that as you're creating something brand new so there is no need
> to keep the existing compat (i.e., __XXX) naming scheme.  In the
> new naming scheme, the internal algorithm should just carry the
> normal alg name (e.g., ecb(aes)) and driver name, while the mcryptd
> wrapped version will have the same algorithm name but carry a
> prefix on the driver name (which is simd- for simd.c, but you
> should probably used mcryptd-).
> 
> Cheers,

I will move this code to the mcryptd.c.

About the naming scheme, could you give me an example where the internal
and external algorithm have the same name? I tried searching but did not
find any.

When the outer and inner algorithm have the same name, I see a crash
when testing using tcrypt. This is because the wrong algortihm (with a
higher priority) is being picked up in __crypto_alg_lookup.  

Inner alg:
Currently:
alg name:__cbc(aes), driver name:__cbc-aes-aesni-mb

expected:
alg name:cbc(aes), driver name: cbc-aes-aesni-mb

Outer alg:
Currently:
alg name:cbc(aes), driver name:cbc-aes-aesni-mb

expected:
alg name:cbc(aes), driver name:mcryptd-cbc-aes-aesni-mb

Thanks,
Megha