Message-ID: <alpine.DEB.2.21.2204142349180.9383@angie.orcam.me.uk>
Date:   Fri, 15 Apr 2022 13:26:48 +0100 (BST)
From:   "Maciej W. Rozycki" <macro@...am.me.uk>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergmann <arnd@...db.de>, Theodore Ts'o <tytso@....edu>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        "David S . Miller" <davem@...emloft.net>,
        Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Johannes Berg <johannes@...solutions.net>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H . Peter Anvin" <hpa@...or.com>, Chris Zankel <chris@...kel.net>,
        Max Filippov <jcmvbkbc@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Dinh Nguyen <dinguyen@...nel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        "open list:BROADCOM NVRAM DRIVER" <linux-mips@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        sparclinux@...r.kernel.org, linux-um@...ts.infradead.org,
        X86 ML <x86@...nel.org>, linux-xtensa@...ux-xtensa.org
Subject: Re: [PATCH v4 04/11] mips: use fallback for random_get_entropy()
 instead of zero

Hi Jason,

> >  It depends on the exact system.  Some have a 32-bit high-resolution 
> > counter in the chipset (arch/mips/kernel/csrc-ioasic.c) giving like 25MHz 
> > resolution, some have nothing but jiffies.
> 
> Alright, so there _are_ machines with no c0 cycles but with a good
> clock. Yet, 25MHz is still less than the cpu cycle, so this c0 random
> ORing trick remains useful perhaps.

 It's not much less than the CPU cycle really, given that the R3k CPUs are 
clocked at up to 40MHz in the systems concerned and likewise the buggy R4k 
CPUs run at up to 60MHz (and mind that their CP0 Count register increments 
at half the clock rate, so the rate is up to 30MHz anyway).  The overhead 
of the calculation is more than that, let alone the latency and issue rate 
of an uncached MMIO access to the chipset register.

 Also the systems I have in mind and that lack a counter in the chipset 
actually can make use of the buggy CP0 timer, because it's only when CP0 
timer interrupts are used that the erratum matters, but they use a DS1287 
RTC interrupt instead unconditionally as the clock event (see the comment 
at the bottom of arch/mips/dec/time.c).  But this has not been factored in 
with `can_use_mips_counter' (should it just check for `mips_hpt_frequency' 
being zero perhaps, meaning the timer interrupt not being used?).

 Thomas, do you happen to know if any of the SGI systems that we support 
had buggy early R4k chips?

> >  It seems like a reasonable idea to me, but the details would have to be 
> > sorted out, because where a chipset high-resolution counter is available 
> > we want to factor it in, and otherwise we need to extract the right bits 
> > from the CP0 Random register, either 13:8 for the R3k or 5:0 for the R4k.
> 
> One thing we could do here that would seemingly cover all the cases
> without losing _that_ much would be:
> 
>     return (random_get_entropy_fallback() << 13) | ((1<<13) - read_c0_random());

 Except this would have to be:

    return (random_get_entropy_fallback() << 14) | ((1<<14) - read_c0_random());

of course, as bit 13 is still one of the active ones in the R3k CP0 Random 
register.

> Or in case the 13 turns out to be wrong on some hardware, we could
> mitigate the effect with:
> 
>     return (random_get_entropy_fallback() << 13) ^ ((1<<13) - read_c0_random());

 There are two variants only of the CP0 Random register that we can ever 
encounter, as it's been de-facto standardised in early 1990s already and 
then written down in the MIPSr1 architecture specification ~2000.  So I 
think it may make sense to actually handle them both explictitly with 
individual calculations, possibly conditionalised on a CONFIG setting or 
`cpu_has_3kex', because kernels that support the two variants of the MMU 
architecture are mutually incompatible.

 Ah, there's that buggy non-compliant JZ4740 chip too.  I guess we can 
figure out how many CP0 Random bits it implements, though it may be worth 
noting that architecturally the register is not required to decrement, so 
again it may be good to double-check how the JZ4740 selects the values 
there.

 I think the check for a buggy CP0 timer in `can_use_mips_counter' should 
also be qualified with !(CONFIG_CPU_MIPS32 || CONFIG_CPU_MIPS64), which 
will reduce the function to a constant 1 for the overwhelming majority of 
systems out there, without a need to refer to CP0 PRId every time.

> As mentioned in the 1/xx patch of this series,
> random_get_entropy_fallback() should call the highest resolution thing.
> We then shave off the least-changing bits and stuff in the
> faster-changing bits from read_c0_random(). Then, in order to keep it
> counting up instead of down, we do the subtraction there.

 Isn't it going to be an issue for an entropy source that the distribution 
of values obtained from the CP0 Random bit-field is not even, that is some 
values from the 6-bit range will never appear?

> What do you think of this plan?

 Otherwise it makes absolute sense to me.

  Maciej

Powered by blists - more mailing lists