Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 1 Jan 2023 17:21:24 +0100
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        tglx@linutronix.de, linux-crypto@vger.kernel.org,
        linux-api@vger.kernel.org, x86@kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
        Carlos O'Donell <carlos@redhat.com>,
        Florian Weimer <fweimer@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>, Jann Horn <jannh@google.com>,
        Christian Brauner <brauner@kernel.org>,
        Samuel Neves <sneves@dei.uc.pt>
Subject: Re: [PATCH v13 7/7] x86: vdso: Wire up getrandom() vDSO
 implementation
Message-ID: <Y7GzBAt3uUhpfEJD@zx2c4.com>
References: <20221221142327.126451-1-Jason@zx2c4.com>
 <20221221142327.126451-8-Jason@zx2c4.com>
 <Y6OWSM18QL977nbC@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y6OWSM18QL977nbC@sol.localdomain>
Precedence: bulk

On Wed, Dec 21, 2022 at 03:27:04PM -0800, Eric Biggers wrote:
> On Wed, Dec 21, 2022 at 03:23:27PM +0100, Jason A. Donenfeld wrote:
> > diff --git a/arch/x86/entry/vdso/vgetrandom-chacha.S b/arch/x86/entry/vdso/vgetrandom-chacha.S
> > new file mode 100644
> > index 000000000000..91fbb7ac7af4
> > --- /dev/null
> > +++ b/arch/x86/entry/vdso/vgetrandom-chacha.S
> > @@ -0,0 +1,177 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> > + */
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/frame.h>
> > +
> > +.section	.rodata.cst16.CONSTANTS, "aM", @progbits, 16
> > +.align 16
> > +CONSTANTS:	.octa 0x6b20657479622d323320646e61707865
> > +.text
> 
> For simplicity, maybe leave off the section mergeability stuff and just have
> plain ".section .rodata"?

I guess nothing is going to get merged anyway, so sure, why not.

> It would be worth mentioning in the function comment that none of the xmm
> registers are callee-save.  That was not obvious to me.  I know that on arm64,
> *kernel* code doesn't need to save/restore NEON registers, so it's not something
> that arch/arm64/crypto/ does.  But, it *is* needed in arm64 userspace code.  So
> I was worried that something similar would apply to x86_64, but it seems not.

I'll add a comment.

> 
> > +	/* state1[0,1,2,3] = state1[0,3,2,1] */
> > +	pshufd		$0x39,state1,state1
> > +	/* state2[0,1,2,3] = state2[1,0,3,2] */
> > +	pshufd		$0x4e,state2,state2
> > +	/* state3[0,1,2,3] = state3[2,1,0,3] */
> > +	pshufd		$0x93,state3,state3
> 
> The comments don't match the pshufd constants.  The code is correct but the
> comments are not.  They should be:

Er, I swapped the endian when writing the comment. The code is fine
though, yea. Fixed, thanks.

> The above sequence of 24 instructions is repeated twice, so maybe it should be a
> macro (".chacha_round"?).

Not really a fan of the indirection when reading for something simple
like this.

Thanks for the review.

Jason