Re: [PATCH 2/5] Replace invocation of weak PRNG in kernel/bpf/core.c

[Amit Klein <aksecurity@xxxxxxxxx>]

On Mon, Dec 12, 2022 at 8:03 PM Yonghong Song <yhs@xxxxxxxx> wrote:
>
>
>
> On 12/11/22 2:16 PM, david.keisarschm@xxxxxxxxxxxxxxx wrote:
> > From: David <david.keisarschm@xxxxxxxxxxxxxxx>
> >
> > We changed the invocation of
> >   prandom_u32_state to get_random_u32.
> >   We deleted the maintained state,
> >   which was a CPU-variable,
> >   since get_random_u32 maintains its own CPU-variable.
> >   We also deleted the state initializer,
> >   since it is not needed anymore.
> >
> > Signed-off-by: David <david.keisarschm@xxxxxxxxxxxxxxx>
> > ---
> >   include/linux/bpf.h   |  1 -
> >   kernel/bpf/core.c     | 13 +------------
> >   kernel/bpf/verifier.c |  2 --
> >   net/core/filter.c     |  1 -
> >   4 files changed, 1 insertion(+), 16 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
[...]
> Please see the discussion here.
> https://lore.kernel.org/bpf/87edtctz8t.fsf@xxxxxxx/
> There is a performance concern with the above change.
>

I see. How about using (in this instance only!) the SipHash-based
solution which was the basis for prandom_u32() starting with commit
c51f8f88d705 (v5.10-rc1) up until commit d4150779e60f (v5.19-rc1)?
Copying the relevant snippets (minus comments, for brevity) from
/lib/random32.c and /include/linux/prandom.h from that era (the 32
random bits are generated by prandom_u32() at the bottom; I didn't
bother with initialization, and I don't know if the per_cpu logic is
needed here, or perhaps some other kind of locking, if any):


#define PRND_SIPROUND(v0, v1, v2, v3) ( \
v0 += v1, v1 = rol32(v1, 5), v2 += v3, v3 = rol32(v3, 8), \
v1 ^= v0, v0 = rol32(v0, 16), v3 ^= v2, \
v0 += v3, v3 = rol32(v3, 7), v2 += v1, v1 = rol32(v1, 13), \
v3 ^= v0, v1 ^= v2, v2 = rol32(v2, 16) \

)

struct siprand_state {
    unsigned long v0;
    unsigned long v1;
    unsigned long v2;
    unsigned long v3;
};

static DEFINE_PER_CPU(struct siprand_state, net_rand_state)
__latent_entropy;   // do we actually need this? -AK

static inline u32 siprand_u32(struct siprand_state *s)
{
    unsigned long v0 = s->v0, v1 = s->v1, v2 = s->v2, v3 = s->v3;

    PRND_SIPROUND(v0, v1, v2, v3);
    PRND_SIPROUND(v0, v1, v2, v3);
    s->v0 = v0;  s->v1 = v1;  s->v2 = v2;  s->v3 = v3;
    return v1 + v3;
}


u32 prandom_u32(void)
{
    struct siprand_state *state = get_cpu_ptr(&net_rand_state);
    u32 res = siprand_u32(state);

    trace_prandom_u32(res);
    put_cpu_ptr(&net_rand_state);
    return res;
}
EXPORT_SYMBOL(prandom_u32);