linux-kernel - Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

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: <ZttP-SU9i6iOyfnG@zx2c4.com>
Date: Fri, 6 Sep 2024 20:54:49 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	Michael Ellerman <mpe@...erman.id.au>,
	Nicholas Piggin <npiggin@...il.com>,
	Naveen N Rao <naveen@...nel.org>,
	Vincenzo Frascino <vincenzo.frascino@....com>,
	Andrei Vagin <avagin@...il.com>
Subject: Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up
 getrandom() vDSO implementation on VDSO32")

On Fri, Sep 06, 2024 at 05:14:43PM +0200, Christophe Leroy wrote:
> 
> 
> Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
> > On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
> > 
> >> On the long run I wonder if we should try to find a more generic
> >> solution for getrandom instead of requiring each architecture to handle
> >> it. On gettimeofday the selection of the right page is embeded in the
> >> generic part, see for instance :
> >>
> >> static __maybe_unused __kernel_old_time_t
> >> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
> >> {
> >> 	__kernel_old_time_t t;
> >>
> >> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
> >> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
> >> 		vd = __arch_get_timens_vdso_data(vd);
> >>
> >> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
> >>
> >> 	if (time)
> >> 		*time = t;
> >>
> >> 	return t;
> >> }
> >>
> >> and powerpc just provides:
> >>
> >> static __always_inline
> >> const struct vdso_data *__arch_get_timens_vdso_data(const struct
> >> vdso_data *vd)
> >> {
> >> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> >> }
> > 
> > It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
> > have this problem at all, because the layout of their vvars doesn't
> > require it. So the vd->clock_mode access is unnecessary.
> > 
> >> Or another solution could be to put random data in a third page that is
> >> always at the same place regardless of timens ?
> > 
> > Maybe that's the easier way, yea. Potentially wasteful, though.
> > 
> 
> Indeed I just looked at Loongarch and that's exactly what they do: they 
> have a third page after the two pages dedicated to TIME for arch 
> specific data, and they have added getrandom data there.
> 
> The third page is common to every process so it won't waste more than a 
> few bytes. It doesn't worry me even on the older boards that only have 
> 32 Mbytes of RAM.
> 
> So yes, I may have a look at that in the future, what we have at the 
> moment is good enough to move forward.

My x86 code is kind of icky for this:

static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
{
        if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
                return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
        return &__vdso_rng_data;
}

Doing the subtraction like that means that this is more clearly correct.
But it also makes the compiler insert two jumps for the branch, and then
reads the addresses of those variables and such.

If I change it to:

static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
{
        if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
                return (void *)&__vdso_rng_data + (3UL << CONFIG_PAGE_SHIFT);
        return &__vdso_rng_data;
}

Then there's a much nicer single `cmov` with no branching.

But if I want to do that for real, I'll have to figure out what set of
nice compile-time constants I can use. I haven't looked into this yet.

Jason