linux-kernel - Re: [PATCH] crypto: lib/chacha20poly1305 - Set SG_MITER

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: <CAMj1kXEhQL-=tDRB8dW6L=EYn1OGjEKW3VKgFRV5O791NdJfzQ@mail.gmail.com>
Date:   Tue, 15 Sep 2020 09:03:46 +0300
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Herbert Xu <herbert@...dor.apana.org.au>,
        "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: [PATCH] crypto: lib/chacha20poly1305 - Set SG_MITER_ATOMIC unconditionally

(+ Jason)

On Tue, 15 Sep 2020 at 06:30, Herbert Xu <herbert@...dor.apana.org.au> wrote:
>
> I trimmed the cc as the mailing lists appear to be blocking this
> email because of it.
>
> On Mon, Sep 14, 2020 at 03:37:49PM -0700, Linus Torvalds wrote:
> >
> > So it _looks_ like this code started using kmap() - probably back when
> > kmap_atomic() was so cumbersome to use - and was then converted
> > (conditionally) to kmap_atomic() rather than just changed whole-sale.
> > Is there actually something that wants to use those sg_miter functions
> > and sleep?
>
> I dug up the old zinc patch submissions and this wasn't present at
> all in the original.  The original zinc code used blkcipher_walk
> which unconditinoally does kmap_atomic.
>

Remember that the Zinc patchset was very vocal about not relying on
the Linux crypto API, yet it [ab]used the crypto blkcipher_walk API
(which was already deprecated at that point) in a rather horrid way,
by going around the blkcipher API itself, and creating some mock
objects that the blkcipher scatterlist walker would expect to exist.

So instead, I opted to rewrite this code using the SG miter API so that:
- src == dst, and so we only need to traverse (and kmap) a single
scatterlist instead of two in parallel (as Wireguard has no need for
the latter)
- no elaborate handling of the scatterlist elements when they are not
a multiple of the cipher chunk size (which is not needed for a stream
cipher liker ChaCha)
- no need to use scatterwalk_map_and_copy() (and do another kmap()) to
access the tag if it was covered by the last scatterlist element.

> So it's only the SG miter conversion that introduced this change,
> which appears to be a simple oversight (I think Ard was working on
> latency issues at that time, perhaps he was worried about keeping
> preemption off unnecessarily).
>

No, the problem with using kmap_atomic() is that it disables
preemption even on !HIGHMEM architectures. So using it unconditionally
here means that all chacha/poly processing will execute with
preemption disabled on 64-bit architectures as well.

This means that, even if you avoid the SIMD accelerated ciphers for
latency reasons (as they disable preemption as well), you are still
running the bulk of the WireGuard processing with preemption disabled.

> ---8<---
> There is no reason for the chacha20poly1305 SG miter code to use
> kmap instead of kmap_atomic as the critical section doesn't sleep
> anyway.  So we can simply get rid of the preemptible check and
> set SG_MITER_ATOMIC unconditionally.
>
> Even if we need to reenable preemption to lower latency we should
> be doing that by interrupting the SG miter walk rather than using
> kmap.
>

AIUI, the common case is that the entire packet is covered by a single
scatterlist element, so there is no room for latency reduction here.

The problem is really that kmap_atomic() is not simply a no-op on
!HIGHMEM architectures. If we can fix that, I have no objections to
this patch.

> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
>
> diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> index 431e04280332..5850f3b87359 100644
> --- a/lib/crypto/chacha20poly1305.c
> +++ b/lib/crypto/chacha20poly1305.c
> @@ -251,9 +251,7 @@ bool chacha20poly1305_crypt_sg_inplace(struct scatterlist *src,
>                         poly1305_update(&poly1305_state, pad0, 0x10 - (ad_len & 0xf));
>         }
>
> -       flags = SG_MITER_TO_SG;
> -       if (!preemptible())
> -               flags |= SG_MITER_ATOMIC;
> +       flags = SG_MITER_TO_SG | SG_MITER_ATOMIC;
>
>         sg_miter_start(&miter, src, sg_nents(src), flags);
>
> --
> Email: Herbert Xu <herbert@...dor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt