linux-kernel - Re: [RFC 01/10] crypto: factor async completion for general use

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]

Date:   Thu, 11 May 2017 10:29:47 +0300
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jonathan Corbet <corbet@....net>,
        David Howells <dhowells@...hat.com>,
        Alasdair Kergon <agk@...hat.com>,
        Mike Snitzer <snitzer@...hat.com>, dm-devel@...hat.com,
        Shaohua Li <shli@...nel.org>, Steve French <sfrench@...ba.org>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Ofir Drang <ofir.drang@....com>,
        Gilad Ben-Yossef <gilad.benyossef@....com>,
        linux-crypto@...r.kernel.org, linux-doc@...r.kernel.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        keyrings@...r.kernel.org, linux-raid@...r.kernel.org,
        linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
        linux-fsdevel@...r.kernel.org,
        linux-ima-devel@...ts.sourceforge.net,
        linux-ima-user@...ts.sourceforge.net,
        linux-security-module@...r.kernel.org
Subject: Re: [RFC 01/10] crypto: factor async completion for general use

Hi Eric,

On Thu, May 11, 2017 at 6:55 AM, Eric Biggers <ebiggers3@...il.com> wrote:
> Hi Gilad,
>
> On Sat, May 06, 2017 at 03:59:50PM +0300, Gilad Ben-Yossef wrote:
>> Invoking a possibly async. crypto op and waiting for completion
>> while correctly handling backlog processing is a common task
>> in the crypto API implementation and outside users of it.
>>
>> This patch re-factors one of the in crypto API implementation in
>> preparation for using it across the board instead of hand
>> rolled versions.
>
> Thanks for doing this!  It annoyed me too that there wasn't a helper function
> for this.  Just a few comments below:
>

Thank you for the review.
I will incorporate the feedback into v2.

...

> With regards to the wait being uninterruptible, I agree that this should be the
> default behavior, because I think users waiting for specific crypto requests are
> generally not prepared to handle the wait actually being interrupted.  After
> interruption the crypto operation will still proceed in the background, and it
> will use buffers which the caller has in many cases already freed.  However, I'd
> suggest taking a close look at anything that was actually doing an interruptible
> wait before, to see whether it was a bug or intentional (or "doesn't matter").
>
> And yes there could always be a crypto_wait_req_interruptible() introduced if
> some users need it.

So this one was a bit of a shocker.  I though the  _interruptible use
sites seemed
wrong in the sense of being needless. However, after reading your feedback and
reviewing the code I'm pretty sure every single one of them (including
the one I've
added in dm-verity-target.c this merge window)  are down right dangerous and
can cause random data corruption... so thanks for pointing this out!

I though of this patch set as a "make the code pretty" for 4.13 kind
of patch set.
Looks like it's a bug fix now, maybe even stable material.

Anyway, I'll roll a v2 and we'll see.

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru