* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Jason A. Donenfeld @ 2024-06-07 16:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, patches, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <874ja73xx7.ffs@tglx>
On Wed, Jun 05, 2024 at 11:03:00PM +0200, Thomas Gleixner wrote:
> Jason!
Thomas!
> Can you please split the required defines into a seperate header
> preferrably in include/vdso/ and include that from crypto/chacha.h
Sure. It only actually uses two straight forward constants from there.
> > + u32 key[CHACHA_KEY_SIZE / sizeof(u32)];
>
> CHACHA_STATE_WORDS ?
Nah, that's for CHACHA_BLOCK_SIZE / sizeof(u32), but here is
CHACHA_KEY_SIZE.
>
> > + };
> > + u8 batch_key[CHACHA_BLOCK_SIZE * 2];
>
> What does the u8 buy here over a simple unsigned int?
>
> > + bool in_use;
It means that the structure can be more compact, because `pos` and the
`in_use` boolean will be closer together.
> > diff --git a/include/vdso/types.h b/include/vdso/types.h
> > new file mode 100644
> > index 000000000000..ce131463aeff
> > --- /dev/null
> > +++ b/include/vdso/types.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> Why does this need an extra header when it's clearly getrandom specific?
> Please put this into getrandom.h
From your followup, I just killed the whole thing and now use u64.
> > +/**
> > + * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
> > + * @rng_info: Describes state of kernel RNG, memory shared with kernel.
> > + * @buffer: Destination buffer to fill with random bytes.
> > + * @len: Size of @buffer in bytes.
> > + * @flags: Zero or more GRND_* flags.
> > + * @opaque_state: Pointer to an opaque state area.
> > + *
> > + * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
> > + * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
> > + * schedule that the kernel's RNG is reseeded. If the kernel's RNG is not ready, then this always
> > + * calls into the syscall.
> > + *
> > + * @opaque_state *must* be allocated using the vgetrandom_alloc() syscall. Unless external locking
> > + * is used, one state must be allocated per thread, as it is not safe to call this function
> > + * concurrently with the same @opaque_state. However, it is safe to call this using the same
> > + * @opaque_state that is shared between main code and signal handling code, within the same thread.
> > + *
> > + * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
> > + */
> > +static __always_inline ssize_t
> > +__cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
> > + unsigned int flags, void *opaque_state)
> > +{
> > + ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
>
> We really need to allow reading almost 2GB of random data in one go?
It's just copying the precise semantics as the syscall by bounding the
requested length. The idea is to make this have basically identical
semantics as the syscall (while being way faster).
> > + /*
> > + * @state->in_use is basic reentrancy protection against this running in a signal handler
> > + * with the same @opaque_state, but obviously not atomic wrt multiple CPUs or more than one
> > + * level of reentrancy. If a signal interrupts this after reading @state->in_use, but before
> > + * writing @state->in_use, there is still no race, because the signal handler will run to
> > + * its completion before returning execution.
>
> Can you please add an explanation that the syscall does not touch the
> state and just fills the buffer?
Will do.
> > + */
> > + in_use = READ_ONCE(state->in_use);
> > + if (unlikely(in_use))
> > + goto fallback_syscall;
> > + WRITE_ONCE(state->in_use, true);
> > +
> > +retry_generation:
> > + /*
> > + * @rng_info->generation must always be read here, as it serializes @state->key with the
> > + * kernel's RNG reseeding schedule.
> > + */
> > + current_generation = READ_ONCE(rng_info->generation);
> > +
> > + /*
> > + * If @state->generation doesn't match the kernel RNG's generation, then it means the
> > + * kernel's RNG has reseeded, and so @state->key is reseeded as well.
> > + */
> > + if (unlikely(state->generation != current_generation)) {
> > + /*
> > + * Write the generation before filling the key, in case of fork. If there is a fork
> > + * just after this line, the two forks will get different random bytes from the
>
> the two forks? You mean the parent and the child, no?
Yes, nice catch, thanks.
> > + * syscall, which is good. However, were this line to occur after the getrandom
> > + * syscall, then both child and parent could have the same bytes and the same
> > + * generation counter, so the fork would not be detected. Therefore, write
> > + * @state->generation before the call to the getrandom syscall.
> > + */
> > + WRITE_ONCE(state->generation, current_generation);
> > +
> > + /* Prevent the syscall from being reordered wrt current_generation. */
> > + barrier();
> > +
> > + /* Reseed @state->key using fresh bytes from the kernel. */
> > + if (getrandom_syscall(state->key, sizeof(state->key), 0) != sizeof(state->key)) {
> > + /*
> > + * If the syscall failed to refresh the key, then @state->key is now
> > + * invalid, so invalidate the generation so that it is not used again, and
> > + * fallback to using the syscall entirely.
> > + */
> > + WRITE_ONCE(state->generation, 0);
> > +
> > + /*
> > + * Set @state->in_use to false only after the last write to @state in the
> > + * line above.
> > + */
> > + WRITE_ONCE(state->in_use, false);
>
> So here you rely on the compiler not reordering vs. WRITE_ONCE(),
> i.e. volatile, but above you have a barrier() to prevent the write being
> reordered vs. the syscall, confused.
>
> But even when the compiler does not reorder, what prevents a weakly
> ordered CPU from doing so?
The issue isn't that this code will race between CPUs -- that's
explicitly disallowed by the design. The issue is that this code is
signal-reentrant. So it's mostly a matter of compiler ordering the
instructions correctly. Then, in addition, the write of
current_generation to state->generation must come before the syscall
fires.
> > + if (!len) {
> > + /* Prevent the loop from being reordered wrt ->generation. */
> > + barrier();
>
> Same question as above.
This came out of discussions here:
https://lore.kernel.org/all/878rjlr85s.fsf@oldenburg.str.redhat.com/
And on IRC with Jann.
> > + /* Refill the batch and then overwrite the key, in order to preserve forward secrecy. */
>
> 'and then overwrite'?
>
> Isn't this overwriting it implicitely because batch_key and key are at
> the same place in the union?
Yes, I'll remove the `then`.
Thanks for the review!
Jason
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Jason A. Donenfeld @ 2024-06-07 15:59 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, patches, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <87v82n2g93.ffs@tglx>
On Thu, Jun 06, 2024 at 12:10:00AM +0200, Thomas Gleixner wrote:
> Jason!
>
> On Wed, Jun 05 2024 at 23:03, Thomas Gleixner wrote:
> > On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> >> + */
> >> +#ifdef CONFIG_64BIT
> >> +typedef u64 vdso_kernel_ulong;
> >> +#else
> >> +typedef u32 vdso_kernel_ulong;
> >> +#endif
> >
> > All of this is pointless because if a 32-bit application runs on a
> > 64-bit kernel it has to use the 64-bit 'generation'. So why on earth do
> > we need magic here for a 32-bit kernel?
> >
> > Just use u64 for both and spare all this voodoo. We're seriously not
> > "optimizing" for 32-bit kernels.
>
> All what happens on a 32-bit kernel is that the RNG will store the
> unsigned long (32bit) generation into a 64bit variable:
>
> smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
>
> As the upper 32bit are always zero, there is no issue vs. load store
> tearing at all. So there is zero benefit for this aside of slightly
> "better" user space code when running on a 32-bit kernel. Who cares?
Oh yea. Okay, great. I was concerned about the tearing, but I guess it's
really a non issue. So I'll just make it a u64 and all of this
complexity can just go away. Thanks for thinking about it in a less
convoluted way than me.
> While staring at this I wonder where the corresponding
> smp_load_acquire() is. I haven't found one in the VDSO code.
> READ_ONCE() is only equivalent on a few architectures.
>
> But, what does that store_release() buy at all? There is zero ordering
> vs. anything in the kernel and neither against user space.
>
> If that smp_store_release() serves a purpose then it really has to be
> extensively documented especially as the kernel itself simply uses
> WRITE/READ_ONCE() for base_rng.generation.
This came up here too: https://lore.kernel.org/all/Y3l6ocn1dTN0+1GK@zx2c4.com/
It's to order the writes to the generation counter and is_ready.
Jason
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Jason A. Donenfeld @ 2024-06-07 15:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <CALCETrUgPwVsMwkxkCyuqBKyqouyejikxxyGuBDxnWWKskYG8A@mail.gmail.com>
On Fri, May 31, 2024 at 04:06:37PM -0700, Andy Lutomirski wrote:
> > On May 28, 2024, at 5:25 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Provide a generic C vDSO getrandom() implementation, which operates on
> > an opaque state returned by vgetrandom_alloc() and produces random bytes
> > the same way as getrandom(). This has a the API signature:
> >
> > ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state);
>
> > +/**
> > + * type vdso_kernel_ulong - unsigned long type that matches kernel's unsigned long
> > + *
> > + * Data shared between userspace and the kernel must operate the same way in both 64-bit code and in
> > + * 32-bit compat code, over the same potentially 64-bit kernel. This type represents the size of an
> > + * unsigned long as used by kernel code. This isn't necessarily the same as an unsigned long as used
> > + * by userspace, however.
>
> Why is this better than using plain u64? It’s certainly more
> complicated. It also rather fundamentally breaks CRIU on 32-bit
> userspace (although CRIU may well be unable to keep vgetrandom working
> after a restore onto a different kernel anyway). Admittedly 32-bit
> userspace is a slowly dying breed, but still.
That came out of this conversation: https://lore.kernel.org/all/878rjs7mcx.fsf@oldenburg.str.redhat.com/
(And I'd like single instruction increments, which means long, not u64
on 32-bit machines.)
> > +{
> > + ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
> > + struct vgetrandom_state *state = opaque_state;
> > + size_t batch_len, nblocks, orig_len = len;
> > + unsigned long current_generation;
> > + void *orig_buffer = buffer;
> > + u32 counter[2] = { 0 };
> > + bool in_use, have_retried = false;
> > +
> > + /* The state must not straddle a page, since pages can be zeroed at any time. */
> > + if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
> > + goto fallback_syscall;
>
> This is weird. Either the provided pointer is valid or it isn’t.
> Reasonable outcomes are a segfault if the pointer is bad or success
> (or fallback if needed for some reason) if the pointer is good. Why
> is there specific code to catch a specific sort of pointer screwup
> here?
I guess I could make it return -EFAULT in this case, rather than
silently succeeding.
Jason
^ permalink raw reply
* Re: [PATCH v16 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jann Horn @ 2024-06-07 15:50 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <CAG48ez0pan8aLGjHtoDdrpiP+e5YrGeuD_RzDXgzUwkUvWYLjA@mail.gmail.com>
On Fri, Jun 7, 2024 at 5:12 PM Jann Horn <jannh@google.com> wrote:
> On Fri, Jun 7, 2024 at 4:35 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Fri, May 31, 2024 at 03:00:26PM +0200, Jann Horn wrote:
> > > On Fri, May 31, 2024 at 2:13 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > On Fri, May 31, 2024 at 12:48:58PM +0200, Jann Horn wrote:
> > > > > On Tue, May 28, 2024 at 2:24 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > > > c) If there's not enough memory to service a page fault, it's not fatal.
> > > > > [...]
> > > > > > @@ -5689,6 +5689,10 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> > > > > >
> > > > > > lru_gen_exit_fault();
> > > > > >
> > > > > > + /* If the mapping is droppable, then errors due to OOM aren't fatal. */
> > > > > > + if (vma->vm_flags & VM_DROPPABLE)
> > > > > > + ret &= ~VM_FAULT_OOM;
> > > > >
> > > > > Can you remind me how this is supposed to work? If we get an OOM
> > > > > error, and the error is not fatal, does that mean we'll just keep
> > > > > hitting the same fault handler over and over again (until we happen to
> > > > > have memory available again I guess)?
> > > >
> > > > Right, it'll just keep retrying. I agree this isn't great, which is why
> > > > in the 2023 patchset, I had additional code to simply skip the faulting
> > > > instruction, and then the userspace code would notice the inconsistency
> > > > and fallback to the syscall. This worked pretty well. But it meant
> > > > decoding the instruction and in general skipping instructions is weird,
> > > > and that made this patchset very very contentious. Since the skipping
> > > > behavior isn't actually required by the /security goals/ of this, I
> > > > figured I'd just drop that. And maybe we can all revisit it together
> > > > sometime down the line. But for now I'm hoping for something a little
> > > > easier to swallow.
> > >
> > > In that case, since we need to be able to populate this memory to make
> > > forward progress, would it make sense to remove the parts of the patch
> > > that treat the allocation as if it was allowed to silently fail (the
> > > "__GFP_NOWARN | __GFP_NORETRY" and the "ret &= ~VM_FAULT_OOM")? I
> > > think that would also simplify this a bit by making this type of
> > > memory a little less special.
> >
> > The whole point, though, is that it needs to not fail or warn. It's
> > memory that can be dropped/zeroed at any moment, and the code is
> > deliberately robust to that.
>
> Sure - but does it have to be more robust than accessing a newly
> allocated piece of memory [which hasn't been populated with anonymous
> pages yet] or bringing a swapped-out page back from swap?
>
> I'm not an expert on OOM handling, but my understanding is that the
> kernel tries _really_ hard to avoid failing low-order GFP_KERNEL
> allocations, with the help of the OOM killer. My understanding is that
> those allocations basically can't fail with a NULL return unless the
> process has already been killed or it is in a memcg_kmem cgroup that
> contains only processes that have been marked as exempt from OOM
> killing. (Or if you're using error injection to explicitly tell the
> kernel to fail the allocation.)
> My understanding is that normal outcomes of an out-of-memory situation
> are things like the OOM killer killing processes (including
> potentially the calling one) to free up memory, or the OOM killer
> panic()ing the whole system as a last resort; but getting a NULL
> return from page_alloc(GFP_KERNEL) without getting killed is not one
> of those outcomes.
Or, from a different angle: You're trying to allocate memory, and you
can't make forward progress until that memory has been allocated
(unless the process is killed). That's what GFP_KERNEL is for. Stuff
like "__GFP_NOWARN | __GFP_NORETRY" is for when you have a backup plan
that lets you make progress (perhaps in a slightly less efficient way,
or by dropping some incoming data, or something like that), and it
hints to the page allocator that it doesn't have to try hard to
reclaim memory if it can't find free memory quickly.
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Jason A. Donenfeld @ 2024-06-07 15:37 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <d3f7e1c5-5945-48c0-b263-e09979a987a3@infradead.org>
On Fri, May 31, 2024 at 12:15:16PM -0700, Randy Dunlap wrote:
>
>
> On 5/28/24 5:19 AM, Jason A. Donenfeld wrote:
> > +/**
> > + * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
> > + * @rng_info: Describes state of kernel RNG, memory shared with kernel.
> > + * @buffer: Destination buffer to fill with random bytes.
> > + * @len: Size of @buffer in bytes.
> > + * @flags: Zero or more GRND_* flags.
> > + * @opaque_state: Pointer to an opaque state area.
> > + *
> > + * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
> > + * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
> > + * schedule that the kernel's RNG is reseeded. If the kernel's RNG is not ready, then this always
> > + * calls into the syscall.
> > + *
> > + * @opaque_state *must* be allocated using the vgetrandom_alloc() syscall. Unless external locking
> > + * is used, one state must be allocated per thread, as it is not safe to call this function
> > + * concurrently with the same @opaque_state. However, it is safe to call this using the same
> > + * @opaque_state that is shared between main code and signal handling code, within the same thread.
> > + *
> > + * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
>
> * Returns:
Ack. Thanks.
^ permalink raw reply
* Re: [PATCH v16 5/5] x86: vdso: Wire up getrandom() vDSO implementation
From: Jason A. Donenfeld @ 2024-06-07 15:32 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, patches, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, Samuel Neves
In-Reply-To: <87y17j2hk7.ffs@tglx>
On Wed, Jun 05, 2024 at 11:41:44PM +0200, Thomas Gleixner wrote:
> On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> > +
> > +static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> > +{
> > + if (__vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>
> Lacks an IS_ENABLED(CONFIG_TIMENS)
Thanks, will fix for v+1.
Jason
^ permalink raw reply
* Re: [PATCH v16 5/5] x86: vdso: Wire up getrandom() vDSO implementation
From: Jason A. Donenfeld @ 2024-06-07 15:30 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, Samuel Neves
In-Reply-To: <2b21f479-cf4f-49a3-b7c8-33f87a1998ac@infradead.org>
On Fri, May 31, 2024 at 12:16:03PM -0700, Randy Dunlap wrote:
>
>
> On 5/28/24 5:19 AM, Jason A. Donenfeld wrote:
> > +/**
> > + * getrandom_syscall - Invoke the getrandom() syscall.
> > + * @buffer: Destination buffer to fill with random bytes.
> > + * @len: Size of @buffer in bytes.
> > + * @flags: Zero or more GRND_* flags.
> > + * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
>
> * Returns:
Ack. Thanks.
Jason
^ permalink raw reply
* Re: [PATCH v16 5/5] x86: vdso: Wire up getrandom() vDSO implementation
From: Jason A. Donenfeld @ 2024-06-07 15:27 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, Samuel Neves
In-Reply-To: <20240531033816.GC6505@sol.localdomain>
On Thu, May 30, 2024 at 08:38:16PM -0700, Eric Biggers wrote:
> On Tue, May 28, 2024 at 02:19:54PM +0200, 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..d79e2bd97598
> > --- /dev/null
> > +++ b/arch/x86/entry/vdso/vgetrandom-chacha.S
> > @@ -0,0 +1,178 @@
> > +// 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, "a"
> > +.align 16
> > +CONSTANTS: .octa 0x6b20657479622d323320646e61707865
> > +.text
> > +
> > +/*
> > + * Very basic SSE2 implementation of ChaCha20. Produces a given positive number
> > + * of blocks of output with a nonce of 0, taking an input key and 8-byte
> > + * counter. Importantly does not spill to the stack. Its arguments are:
> > + *
> > + * rdi: output bytes
> > + * rsi: 32-byte key input
> > + * rdx: 8-byte counter input/output
> > + * rcx: number of 64-byte blocks to write to output
> > + */
> > +SYM_FUNC_START(__arch_chacha20_blocks_nostack)
> > +
> > +.set output, %rdi
> > +.set key, %rsi
> > +.set counter, %rdx
> > +.set nblocks, %rcx
> > +.set i, %al
> > +/* xmm registers are *not* callee-save. */
> > +.set state0, %xmm0
> > +.set state1, %xmm1
> > +.set state2, %xmm2
> > +.set state3, %xmm3
> > +.set copy0, %xmm4
> > +.set copy1, %xmm5
> > +.set copy2, %xmm6
> > +.set copy3, %xmm7
> > +.set temp, %xmm8
> > +.set one, %xmm9
>
> An "interesting" x86_64 quirk: in SSE instructions, registers xmm0-xmm7 take
> fewer bytes to encode than xmm8-xmm15.
>
> Since 'temp' is used frequently, moving it into the lower range (and moving one
> of the 'copy' registers, which isn't used as frequently, into the higher range)
> decreases the code size of __arch_chacha20_blocks_nostack() by 5%.
That's a nice trick. Thank you very much for it.
Jason
^ permalink raw reply
* Re: [PATCH v16 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jann Horn @ 2024-06-07 15:12 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <ZmMamtll1Yq1yfxc@zx2c4.com>
On Fri, Jun 7, 2024 at 4:35 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Fri, May 31, 2024 at 03:00:26PM +0200, Jann Horn wrote:
> > On Fri, May 31, 2024 at 2:13 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > On Fri, May 31, 2024 at 12:48:58PM +0200, Jann Horn wrote:
> > > > On Tue, May 28, 2024 at 2:24 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > > c) If there's not enough memory to service a page fault, it's not fatal.
> > > > [...]
> > > > > @@ -5689,6 +5689,10 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> > > > >
> > > > > lru_gen_exit_fault();
> > > > >
> > > > > + /* If the mapping is droppable, then errors due to OOM aren't fatal. */
> > > > > + if (vma->vm_flags & VM_DROPPABLE)
> > > > > + ret &= ~VM_FAULT_OOM;
> > > >
> > > > Can you remind me how this is supposed to work? If we get an OOM
> > > > error, and the error is not fatal, does that mean we'll just keep
> > > > hitting the same fault handler over and over again (until we happen to
> > > > have memory available again I guess)?
> > >
> > > Right, it'll just keep retrying. I agree this isn't great, which is why
> > > in the 2023 patchset, I had additional code to simply skip the faulting
> > > instruction, and then the userspace code would notice the inconsistency
> > > and fallback to the syscall. This worked pretty well. But it meant
> > > decoding the instruction and in general skipping instructions is weird,
> > > and that made this patchset very very contentious. Since the skipping
> > > behavior isn't actually required by the /security goals/ of this, I
> > > figured I'd just drop that. And maybe we can all revisit it together
> > > sometime down the line. But for now I'm hoping for something a little
> > > easier to swallow.
> >
> > In that case, since we need to be able to populate this memory to make
> > forward progress, would it make sense to remove the parts of the patch
> > that treat the allocation as if it was allowed to silently fail (the
> > "__GFP_NOWARN | __GFP_NORETRY" and the "ret &= ~VM_FAULT_OOM")? I
> > think that would also simplify this a bit by making this type of
> > memory a little less special.
>
> The whole point, though, is that it needs to not fail or warn. It's
> memory that can be dropped/zeroed at any moment, and the code is
> deliberately robust to that.
Sure - but does it have to be more robust than accessing a newly
allocated piece of memory [which hasn't been populated with anonymous
pages yet] or bringing a swapped-out page back from swap?
I'm not an expert on OOM handling, but my understanding is that the
kernel tries _really_ hard to avoid failing low-order GFP_KERNEL
allocations, with the help of the OOM killer. My understanding is that
those allocations basically can't fail with a NULL return unless the
process has already been killed or it is in a memcg_kmem cgroup that
contains only processes that have been marked as exempt from OOM
killing. (Or if you're using error injection to explicitly tell the
kernel to fail the allocation.)
My understanding is that normal outcomes of an out-of-memory situation
are things like the OOM killer killing processes (including
potentially the calling one) to free up memory, or the OOM killer
panic()ing the whole system as a last resort; but getting a NULL
return from page_alloc(GFP_KERNEL) without getting killed is not one
of those outcomes.
^ permalink raw reply
* Re: [PATCH v16 2/5] random: add vgetrandom_alloc() syscall
From: Jason A. Donenfeld @ 2024-06-07 14:45 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <ZmMcFonXPVtU0moO@zx2c4.com>
On Fri, Jun 07, 2024 at 04:41:26PM +0200, Jason A. Donenfeld wrote:
> On Tue, Jun 04, 2024 at 10:22:49AM -0700, Eric Biggers wrote:
> > On Sat, Jun 01, 2024 at 12:56:40PM +0200, Jason A. Donenfeld wrote:
> > > On Thu, May 30, 2024 at 08:59:17PM -0700, Eric Biggers wrote:
> > > > On Tue, May 28, 2024 at 02:19:51PM +0200, Jason A. Donenfeld wrote:
> > > > > +/**
> > > > > + * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
> > > > > + *
> > > > > + * @num: On input, a pointer to a suggested hint of how many states to
> > > > > + * allocate, and on return the number of states actually allocated.
> > > > > + *
> > > > > + * @size_per_each: On input, must be zero. On return, the size of each state allocated,
> > > > > + * so that the caller can split up the returned allocation into
> > > > > + * individual states.
> > > > > + *
> > > > > + * @addr: Reserved, must be zero.
> > > > > + *
> > > > > + * @flags: Reserved, must be zero.
> > > > > + *
> > > > > + * The getrandom() vDSO function in userspace requires an opaque state, which
> > > > > + * this function allocates by mapping a certain number of special pages into
> > > > > + * the calling process. It takes a hint as to the number of opaque states
> > > > > + * desired, and provides the caller with the number of opaque states actually
> > > > > + * allocated, the size of each one in bytes, and the address of the first
> > > > > + * state, which may be split up into @num states of @size_per_each bytes each,
> > > > > + * by adding @size_per_each to the returned first state @num times, while
> > > > > + * ensuring that no single state straddles a page boundary.
> > > > > + *
> > > > > + * Returns the address of the first state in the allocation on success, or a
> > > > > + * negative error value on failure.
> > > > > + *
> > > > > + * The returned address of the first state may be passed to munmap(2) with a
> > > > > + * length of `(size_t)num * (size_t)size_per_each`, in order to deallocate the
> > > > > + * memory, after which it is invalid to pass it to vDSO getrandom().
> > > >
> > > > Wouldn't a munmap with '(size_t)num * (size_t)size_per_each' be potentially too
> > > > short, due to how the allocation is sized such that states don't cross page
> > > > boundaries?
> > >
> > > You're right, I think. The calculation should instead be something like:
> > >
> > > DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE
> > >
> > > Does that seem correct to you?
> > >
> >
> > Yes, though I wonder if it would be better to give userspace the number of pages
> > instead of the number of states.
>
> Or maybe just the number of total bytes allocated? That would match
> what's expected to be passed to munmap() and is maybe the easiest to
> deal with. I'll give that a shot for v+1.
Hmm, though, on second thought,
* @num: On input, a pointer to a suggested hint of how many states to
* allocate, and on return the number of states actually allocated.
This is kind of elegant -- it's an in/out param. Changing the semantics
of the out param isn't super obvious. And bytes means it should probably
be a long too. So maybe I'll keep it as is, and fix the documentation to
have the right calculation.
Jason
^ permalink raw reply
* Re: [PATCH v16 2/5] random: add vgetrandom_alloc() syscall
From: Jason A. Donenfeld @ 2024-06-07 14:41 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240604172249.GA1566@sol.localdomain>
On Tue, Jun 04, 2024 at 10:22:49AM -0700, Eric Biggers wrote:
> On Sat, Jun 01, 2024 at 12:56:40PM +0200, Jason A. Donenfeld wrote:
> > On Thu, May 30, 2024 at 08:59:17PM -0700, Eric Biggers wrote:
> > > On Tue, May 28, 2024 at 02:19:51PM +0200, Jason A. Donenfeld wrote:
> > > > +/**
> > > > + * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
> > > > + *
> > > > + * @num: On input, a pointer to a suggested hint of how many states to
> > > > + * allocate, and on return the number of states actually allocated.
> > > > + *
> > > > + * @size_per_each: On input, must be zero. On return, the size of each state allocated,
> > > > + * so that the caller can split up the returned allocation into
> > > > + * individual states.
> > > > + *
> > > > + * @addr: Reserved, must be zero.
> > > > + *
> > > > + * @flags: Reserved, must be zero.
> > > > + *
> > > > + * The getrandom() vDSO function in userspace requires an opaque state, which
> > > > + * this function allocates by mapping a certain number of special pages into
> > > > + * the calling process. It takes a hint as to the number of opaque states
> > > > + * desired, and provides the caller with the number of opaque states actually
> > > > + * allocated, the size of each one in bytes, and the address of the first
> > > > + * state, which may be split up into @num states of @size_per_each bytes each,
> > > > + * by adding @size_per_each to the returned first state @num times, while
> > > > + * ensuring that no single state straddles a page boundary.
> > > > + *
> > > > + * Returns the address of the first state in the allocation on success, or a
> > > > + * negative error value on failure.
> > > > + *
> > > > + * The returned address of the first state may be passed to munmap(2) with a
> > > > + * length of `(size_t)num * (size_t)size_per_each`, in order to deallocate the
> > > > + * memory, after which it is invalid to pass it to vDSO getrandom().
> > >
> > > Wouldn't a munmap with '(size_t)num * (size_t)size_per_each' be potentially too
> > > short, due to how the allocation is sized such that states don't cross page
> > > boundaries?
> >
> > You're right, I think. The calculation should instead be something like:
> >
> > DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE
> >
> > Does that seem correct to you?
> >
>
> Yes, though I wonder if it would be better to give userspace the number of pages
> instead of the number of states.
Or maybe just the number of total bytes allocated? That would match
what's expected to be passed to munmap() and is maybe the easiest to
deal with. I'll give that a shot for v+1.
Jason
^ permalink raw reply
* Re: [PATCH v16 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-06-07 14:35 UTC (permalink / raw)
To: Jann Horn
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <CAG48ez3VhWpJnzHHn4NAJdrsd1Ts9hs0zvHa6Pqwatu4wV63Kw@mail.gmail.com>
On Fri, May 31, 2024 at 03:00:26PM +0200, Jann Horn wrote:
> On Fri, May 31, 2024 at 2:13 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Fri, May 31, 2024 at 12:48:58PM +0200, Jann Horn wrote:
> > > On Tue, May 28, 2024 at 2:24 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > c) If there's not enough memory to service a page fault, it's not fatal.
> > > [...]
> > > > @@ -5689,6 +5689,10 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> > > >
> > > > lru_gen_exit_fault();
> > > >
> > > > + /* If the mapping is droppable, then errors due to OOM aren't fatal. */
> > > > + if (vma->vm_flags & VM_DROPPABLE)
> > > > + ret &= ~VM_FAULT_OOM;
> > >
> > > Can you remind me how this is supposed to work? If we get an OOM
> > > error, and the error is not fatal, does that mean we'll just keep
> > > hitting the same fault handler over and over again (until we happen to
> > > have memory available again I guess)?
> >
> > Right, it'll just keep retrying. I agree this isn't great, which is why
> > in the 2023 patchset, I had additional code to simply skip the faulting
> > instruction, and then the userspace code would notice the inconsistency
> > and fallback to the syscall. This worked pretty well. But it meant
> > decoding the instruction and in general skipping instructions is weird,
> > and that made this patchset very very contentious. Since the skipping
> > behavior isn't actually required by the /security goals/ of this, I
> > figured I'd just drop that. And maybe we can all revisit it together
> > sometime down the line. But for now I'm hoping for something a little
> > easier to swallow.
>
> In that case, since we need to be able to populate this memory to make
> forward progress, would it make sense to remove the parts of the patch
> that treat the allocation as if it was allowed to silently fail (the
> "__GFP_NOWARN | __GFP_NORETRY" and the "ret &= ~VM_FAULT_OOM")? I
> think that would also simplify this a bit by making this type of
> memory a little less special.
The whole point, though, is that it needs to not fail or warn. It's
memory that can be dropped/zeroed at any moment, and the code is
deliberately robust to that.
Jason
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Thomas Gleixner @ 2024-06-05 22:10 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, patches
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <874ja73xx7.ffs@tglx>
Jason!
On Wed, Jun 05 2024 at 23:03, Thomas Gleixner wrote:
> On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
>> + */
>> +#ifdef CONFIG_64BIT
>> +typedef u64 vdso_kernel_ulong;
>> +#else
>> +typedef u32 vdso_kernel_ulong;
>> +#endif
>
> All of this is pointless because if a 32-bit application runs on a
> 64-bit kernel it has to use the 64-bit 'generation'. So why on earth do
> we need magic here for a 32-bit kernel?
>
> Just use u64 for both and spare all this voodoo. We're seriously not
> "optimizing" for 32-bit kernels.
All what happens on a 32-bit kernel is that the RNG will store the
unsigned long (32bit) generation into a 64bit variable:
smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
As the upper 32bit are always zero, there is no issue vs. load store
tearing at all. So there is zero benefit for this aside of slightly
"better" user space code when running on a 32-bit kernel. Who cares?
While staring at this I wonder where the corresponding
smp_load_acquire() is. I haven't found one in the VDSO code.
READ_ONCE() is only equivalent on a few architectures.
But, what does that store_release() buy at all? There is zero ordering
vs. anything in the kernel and neither against user space.
If that smp_store_release() serves a purpose then it really has to be
extensively documented especially as the kernel itself simply uses
WRITE/READ_ONCE() for base_rng.generation.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v16 5/5] x86: vdso: Wire up getrandom() vDSO implementation
From: Thomas Gleixner @ 2024-06-05 21:41 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, patches
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, Samuel Neves
In-Reply-To: <20240528122352.2485958-6-Jason@zx2c4.com>
On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> +
> +static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> +{
> + if (__vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
Lacks an IS_ENABLED(CONFIG_TIMENS)
> + return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
> + return &__vdso_rng_data;
> +}
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Thomas Gleixner @ 2024-06-05 21:03 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, patches
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240528122352.2485958-5-Jason@zx2c4.com>
Jason!
On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
> index e3ceb1976386..7dc93d5f72dc 100644
> --- a/include/vdso/getrandom.h
> +++ b/include/vdso/getrandom.h
> @@ -6,11 +6,39 @@
> #ifndef _VDSO_GETRANDOM_H
> #define _VDSO_GETRANDOM_H
>
> +#include <crypto/chacha.h>
Can you please split the required defines into a seperate header
preferrably in include/vdso/ and include that from crypto/chacha.h
The point is that VDSO is very intentionally not using anything outside
include/uapi/ and include/vdso/ except for include/linux/compiler.h and
include/linux/types.h.
We've had too much trouble of random include chains which magically
break the build dependent on architectures and configurations. VDSO is a userspace
library after all.
> +#include <vdso/types.h>
> +
> /**
> * struct vgetrandom_state - State used by vDSO getrandom() and allocated by vgetrandom_alloc().
> *
> - * Currently empty, as the vDSO getrandom() function has not yet been implemented.
> + * @batch: One and a half ChaCha20 blocks of buffered RNG output.
> + *
> + * @key: Key to be used for generating next batch.
> + *
> + * @batch_key: Union of the prior two members, which is exactly two full
> + * ChaCha20 blocks in size, so that @batch and @key can be filled
> + * together.
> + *
> + * @generation: Snapshot of @rng_info->generation in the vDSO data page at
> + * the time @key was generated.
> + *
> + * @pos: Offset into @batch of the next available random byte.
> + *
> + * @in_use: Reentrancy guard for reusing a state within the same thread
> + * due to signal handlers.
> */
> -struct vgetrandom_state { int placeholder; };
> +struct vgetrandom_state {
> + union {
> + struct {
> + u8 batch[CHACHA_BLOCK_SIZE * 3 / 2];
> + u32 key[CHACHA_KEY_SIZE / sizeof(u32)];
CHACHA_STATE_WORDS ?
> + };
> + u8 batch_key[CHACHA_BLOCK_SIZE * 2];
Lot's of magic constants here *3/2 *2 ....
> + };
> + vdso_kernel_ulong generation;
> + u8 pos;
What does the u8 buy here over a simple unsigned int?
> + bool in_use;
> +};
>
> #endif /* _VDSO_GETRANDOM_H */
> diff --git a/include/vdso/types.h b/include/vdso/types.h
> new file mode 100644
> index 000000000000..ce131463aeff
> --- /dev/null
> +++ b/include/vdso/types.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
Why does this need an extra header when it's clearly getrandom specific?
Please put this into getrandom.h
> +/*
> + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +#ifndef __VDSO_TYPES_H
> +#define __VDSO_TYPES_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * type vdso_kernel_ulong - unsigned long type that matches kernel's unsigned long
> + *
> + * Data shared between userspace and the kernel must operate the same way in both 64-bit code and in
> + * 32-bit compat code, over the same potentially 64-bit kernel. This type represents the size of an
> + * unsigned long as used by kernel code. This isn't necessarily the same as an unsigned long as used
> + * by userspace, however.
This is confusing at best.
First of all 64-bit code can run only on a 64-bit kernel, so what does
'the same potentially 64-bit kernel' even mean in that sentence?
What means: 'This type represents the size of an unsigned long as used by kernel
code'?
> + * +-------------------+-------------------+------------------+-------------------+
> + * | 32-bit userspace | 32-bit userspace | 64-bit userspace | 64-bit userspace |
> + * | unsigned long | vdso_kernel_ulong | unsigned long | vdso_kernel_ulong |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
> + * | 32-bit kernel | ✓ same size | ✓ same size |
> + * | unsigned long | | |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
> + * | 64-bit kernel | ✘ different size! | ✓ same size | ✓ same size | ✓ same size |
> + * | unsigned long | | | | |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
I have no idea what this table tries to tell me, but I clearly can see
what you are trying to achieve here:
> + */
> +#ifdef CONFIG_64BIT
> +typedef u64 vdso_kernel_ulong;
> +#else
> +typedef u32 vdso_kernel_ulong;
> +#endif
All of this is pointless because if a 32-bit application runs on a
64-bit kernel it has to use the 64-bit 'generation'. So why on earth do
we need magic here for a 32-bit kernel?
Just use u64 for both and spare all this voodoo. We're seriously not
"optimizing" for 32-bit kernels.
> +/**
> + * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
> + * @rng_info: Describes state of kernel RNG, memory shared with kernel.
> + * @buffer: Destination buffer to fill with random bytes.
> + * @len: Size of @buffer in bytes.
> + * @flags: Zero or more GRND_* flags.
> + * @opaque_state: Pointer to an opaque state area.
> + *
> + * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
> + * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
> + * schedule that the kernel's RNG is reseeded. If the kernel's RNG is not ready, then this always
> + * calls into the syscall.
> + *
> + * @opaque_state *must* be allocated using the vgetrandom_alloc() syscall. Unless external locking
> + * is used, one state must be allocated per thread, as it is not safe to call this function
> + * concurrently with the same @opaque_state. However, it is safe to call this using the same
> + * @opaque_state that is shared between main code and signal handling code, within the same thread.
> + *
> + * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
> + */
> +static __always_inline ssize_t
> +__cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
> + unsigned int flags, void *opaque_state)
> +{
> + ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
We really need to allow reading almost 2GB of random data in one go?
> + struct vgetrandom_state *state = opaque_state;
> + size_t batch_len, nblocks, orig_len = len;
> + unsigned long current_generation;
> + void *orig_buffer = buffer;
> + u32 counter[2] = { 0 };
> + bool in_use, have_retried = false;
Please keep the reverse fir tree order.
> + /* The state must not straddle a page, since pages can be zeroed at any time. */
> + if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
> + goto fallback_syscall;
> +
> + /*
> + * If the kernel's RNG is not yet ready, then it's not possible to provide random bytes from
> + * userspace, because A) the various @flags require this to block, or not, depending on
> + * various factors unavailable to userspace, and B) the kernel's behavior before the RNG is
> + * ready is to reseed from the entropy pool at every invocation.
> + */
> + if (unlikely(!READ_ONCE(rng_info->is_ready)))
> + goto fallback_syscall;
> +
> + /*
> + * This condition is checked after @rng_info->is_ready, because before the kernel's RNG is
> + * initialized, the @flags parameter may require this to block or return an error, even when
> + * len is zero.
> + */
> + if (unlikely(!len))
> + return 0;
> +
> + /*
> + * @state->in_use is basic reentrancy protection against this running in a signal handler
> + * with the same @opaque_state, but obviously not atomic wrt multiple CPUs or more than one
> + * level of reentrancy. If a signal interrupts this after reading @state->in_use, but before
> + * writing @state->in_use, there is still no race, because the signal handler will run to
> + * its completion before returning execution.
Can you please add an explanation that the syscall does not touch the
state and just fills the buffer?
> + */
> + in_use = READ_ONCE(state->in_use);
> + if (unlikely(in_use))
> + goto fallback_syscall;
> + WRITE_ONCE(state->in_use, true);
> +
> +retry_generation:
> + /*
> + * @rng_info->generation must always be read here, as it serializes @state->key with the
> + * kernel's RNG reseeding schedule.
> + */
> + current_generation = READ_ONCE(rng_info->generation);
> +
> + /*
> + * If @state->generation doesn't match the kernel RNG's generation, then it means the
> + * kernel's RNG has reseeded, and so @state->key is reseeded as well.
> + */
> + if (unlikely(state->generation != current_generation)) {
> + /*
> + * Write the generation before filling the key, in case of fork. If there is a fork
> + * just after this line, the two forks will get different random bytes from the
the two forks? You mean the parent and the child, no?
> + * syscall, which is good. However, were this line to occur after the getrandom
> + * syscall, then both child and parent could have the same bytes and the same
> + * generation counter, so the fork would not be detected. Therefore, write
> + * @state->generation before the call to the getrandom syscall.
> + */
> + WRITE_ONCE(state->generation, current_generation);
> +
> + /* Prevent the syscall from being reordered wrt current_generation. */
> + barrier();
> +
> + /* Reseed @state->key using fresh bytes from the kernel. */
> + if (getrandom_syscall(state->key, sizeof(state->key), 0) != sizeof(state->key)) {
> + /*
> + * If the syscall failed to refresh the key, then @state->key is now
> + * invalid, so invalidate the generation so that it is not used again, and
> + * fallback to using the syscall entirely.
> + */
> + WRITE_ONCE(state->generation, 0);
> +
> + /*
> + * Set @state->in_use to false only after the last write to @state in the
> + * line above.
> + */
> + WRITE_ONCE(state->in_use, false);
So here you rely on the compiler not reordering vs. WRITE_ONCE(),
i.e. volatile, but above you have a barrier() to prevent the write being
reordered vs. the syscall, confused.
But even when the compiler does not reorder, what prevents a weakly
ordered CPU from doing so?
> + goto fallback_syscall;
> + }
> +
> + /*
> + * Set @state->pos to beyond the end of the batch, so that the batch is refilled
> + * using the new key.
> + */
> + state->pos = sizeof(state->batch);
> + }
> +
> + /* Set len to the total amount of bytes that this function is allowed to read, ret. */
> + len = ret;
> +more_batch:
> + /*
> + * First use bytes out of @state->batch, which may have been filled by the last call to this
> + * function.
> + */
> + batch_len = min_t(size_t, sizeof(state->batch) - state->pos, len);
> + if (batch_len) {
> + /* Zeroing at the same time as memcpying helps preserve forward secrecy. */
> + memcpy_and_zero_src(buffer, state->batch + state->pos, batch_len);
> + state->pos += batch_len;
> + buffer += batch_len;
> + len -= batch_len;
> + }
> +
> + if (!len) {
> + /* Prevent the loop from being reordered wrt ->generation. */
> + barrier();
Same question as above.
> + /*
> + * Since @rng_info->generation will never be 0, re-read @state->generation, rather
> + * than using the local current_generation variable, to learn whether a fork
> + * occurred or if @state was zeroed due to memory pressure. Primarily, though, this
> + * indicates whether the kernel's RNG has reseeded, in which case generate a new key
> + * and start over.
> + */
> + if (unlikely(READ_ONCE(state->generation) != READ_ONCE(rng_info->generation))) {
> + /*
> + * Prevent this from looping forever in case of low memory or racing with a
> + * user force-reseeding the kernel's RNG using the ioctl.
> + */
> + if (have_retried) {
> + WRITE_ONCE(state->in_use, false);
> + goto fallback_syscall;
> + }
> +
> + have_retried = true;
> + buffer = orig_buffer;
> + goto retry_generation;
> + }
> +
> + /*
> + * Set @state->in_use to false only when there will be no more reads or writes of
> + * @state.
> + */
> + WRITE_ONCE(state->in_use, false);
> + return ret;
> + }
> +
> + /* Generate blocks of RNG output directly into @buffer while there's enough room left. */
> + nblocks = len / CHACHA_BLOCK_SIZE;
> + if (nblocks) {
> + __arch_chacha20_blocks_nostack(buffer, state->key, counter, nblocks);
> + buffer += nblocks * CHACHA_BLOCK_SIZE;
> + len -= nblocks * CHACHA_BLOCK_SIZE;
> + }
> +
> + BUILD_BUG_ON(sizeof(state->batch_key) % CHACHA_BLOCK_SIZE != 0);
> +
> + /* Refill the batch and then overwrite the key, in order to preserve forward secrecy. */
'and then overwrite'?
Isn't this overwriting it implicitely because batch_key and key are at
the same place in the union?
> + __arch_chacha20_blocks_nostack(state->batch_key, state->key, counter,
> + sizeof(state->batch_key) / CHACHA_BLOCK_SIZE);
> +
> + /* Since the batch was just refilled, set the position back to 0 to indicate a full batch. */
> + state->pos = 0;
> + goto more_batch;
Thanks,
tglx
^ permalink raw reply
* Re: [PATCHv7 bpf-next 0/9] uprobe: uretprobe speed up
From: Andrii Nakryiko @ 2024-06-05 16:42 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Oleg Nesterov, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, linux-kernel, linux-trace-kernel, linux-api,
linux-man, x86, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD),
Ingo Molnar, Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta,
Linus Torvalds
In-Reply-To: <CAEf4Bza-+=04GG7Tg4U4pCQ28Oy_2F_5872EPDsX6X3Y=jhEuw@mail.gmail.com>
On Fri, May 31, 2024 at 10:52 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 23, 2024 at 5:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > as part of the effort on speeding up the uprobes [0] coming with
> > return uprobe optimization by using syscall instead of the trap
> > on the uretprobe trampoline.
> >
> > The speed up depends on instruction type that uprobe is installed
> > and depends on specific HW type, please check patch 1 for details.
> >
> > Patches 1-8 are based on bpf-next/master, but patch 2 and 3 are
> > apply-able on linux-trace.git tree probes/for-next branch.
> > Patch 9 is based on man-pages master.
> >
> > v7 changes:
> > - fixes in man page [Alejandro Colomar]
> > - fixed patch #1 fixes tag [Oleg]
> >
> > Also available at:
> > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > uretprobe_syscall
> >
> > thanks,
> > jirka
> >
> >
> > Notes to check list items in Documentation/process/adding-syscalls.rst:
> >
> > - System Call Alternatives
> > New syscall seems like the best way in here, because we need
> > just to quickly enter kernel with no extra arguments processing,
> > which we'd need to do if we decided to use another syscall.
> >
> > - Designing the API: Planning for Extension
> > The uretprobe syscall is very specific and most likely won't be
> > extended in the future.
> >
> > At the moment it does not take any arguments and even if it does
> > in future, it's allowed to be called only from trampoline prepared
> > by kernel, so there'll be no broken user.
> >
> > - Designing the API: Other Considerations
> > N/A because uretprobe syscall does not return reference to kernel
> > object.
> >
> > - Proposing the API
> > Wiring up of the uretprobe system call is in separate change,
> > selftests and man page changes are part of the patchset.
> >
> > - Generic System Call Implementation
> > There's no CONFIG option for the new functionality because it
> > keeps the same behaviour from the user POV.
> >
> > - x86 System Call Implementation
> > It's 64-bit syscall only.
> >
> > - Compatibility System Calls (Generic)
> > N/A uretprobe syscall has no arguments and is not supported
> > for compat processes.
> >
> > - Compatibility System Calls (x86)
> > N/A uretprobe syscall is not supported for compat processes.
> >
> > - System Calls Returning Elsewhere
> > N/A.
> >
> > - Other Details
> > N/A.
> >
> > - Testing
> > Adding new bpf selftests and ran ltp on top of this change.
> >
> > - Man Page
> > Attached.
> >
> > - Do not call System Calls in the Kernel
> > N/A.
> >
> >
> > [0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/
> > ---
> > Jiri Olsa (8):
> > x86/shstk: Make return uprobe work with shadow stack
> > uprobe: Wire up uretprobe system call
> > uprobe: Add uretprobe syscall to speed up return probe
> > selftests/x86: Add return uprobe shadow stack test
> > selftests/bpf: Add uretprobe syscall test for regs integrity
> > selftests/bpf: Add uretprobe syscall test for regs changes
> > selftests/bpf: Add uretprobe syscall call from user space test
> > selftests/bpf: Add uretprobe shadow stack test
> >
>
> Masami, Steven,
>
> It seems like the series is ready to go in. Are you planning to take
> the first 4 patches through your linux-trace tree?
Another ping. It's been two weeks since Jiri posted the last revision
that got no more feedback to be addressed and everyone seems to be
happy with it.
This is an important speed up improvement for uprobe infrastructure in
general and for BPF ecosystem in particular. "Uprobes are slow" is one
of the top complaints from production BPF users, and sys_uretprobe
approach is significantly improving the situation for return uprobes
(aka uretprobes), potentially enabling new use cases that previously
could have been too expensive to trace in practice and reducing the
overhead of the existing ones.
I'd appreciate the engagement from linux-trace maintainers on this
patch set. Given it's important for BPF and that a big part of the
patch set is BPF-based selftests, we'd also be happy to route all this
through the bpf-next tree (which would actually make logistics for us
much easier, but that's not the main concern). But regardless of the
tree, it would be nice to make a decision and go forward with it.
Thank you!
>
> > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> > arch/x86/include/asm/shstk.h | 4 +
> > arch/x86/kernel/shstk.c | 16 ++++
> > arch/x86/kernel/uprobes.c | 124 ++++++++++++++++++++++++++++-
> > include/linux/syscalls.h | 2 +
> > include/linux/uprobes.h | 3 +
> > include/uapi/asm-generic/unistd.h | 5 +-
> > kernel/events/uprobes.c | 24 ++++--
> > kernel/sys_ni.c | 2 +
> > tools/include/linux/compiler.h | 4 +
> > tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 123 ++++++++++++++++++++++++++++-
> > tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 385 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > tools/testing/selftests/bpf/progs/uprobe_syscall.c | 15 ++++
> > tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c | 17 ++++
> > tools/testing/selftests/x86/test_shadow_stack.c | 145 ++++++++++++++++++++++++++++++++++
> > 15 files changed, 860 insertions(+), 10 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
> >
> > Jiri Olsa (1):
> > man2: Add uretprobe syscall page
> >
> > man/man2/uretprobe.2 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> > create mode 100644 man/man2/uretprobe.2
^ permalink raw reply
* Re: [PATCH v16 2/5] random: add vgetrandom_alloc() syscall
From: Eric Biggers @ 2024-06-04 17:22 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <Zlr-aMJdAEgHOj9-@zx2c4.com>
On Sat, Jun 01, 2024 at 12:56:40PM +0200, Jason A. Donenfeld wrote:
> On Thu, May 30, 2024 at 08:59:17PM -0700, Eric Biggers wrote:
> > On Tue, May 28, 2024 at 02:19:51PM +0200, Jason A. Donenfeld wrote:
> > > +/**
> > > + * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
> > > + *
> > > + * @num: On input, a pointer to a suggested hint of how many states to
> > > + * allocate, and on return the number of states actually allocated.
> > > + *
> > > + * @size_per_each: On input, must be zero. On return, the size of each state allocated,
> > > + * so that the caller can split up the returned allocation into
> > > + * individual states.
> > > + *
> > > + * @addr: Reserved, must be zero.
> > > + *
> > > + * @flags: Reserved, must be zero.
> > > + *
> > > + * The getrandom() vDSO function in userspace requires an opaque state, which
> > > + * this function allocates by mapping a certain number of special pages into
> > > + * the calling process. It takes a hint as to the number of opaque states
> > > + * desired, and provides the caller with the number of opaque states actually
> > > + * allocated, the size of each one in bytes, and the address of the first
> > > + * state, which may be split up into @num states of @size_per_each bytes each,
> > > + * by adding @size_per_each to the returned first state @num times, while
> > > + * ensuring that no single state straddles a page boundary.
> > > + *
> > > + * Returns the address of the first state in the allocation on success, or a
> > > + * negative error value on failure.
> > > + *
> > > + * The returned address of the first state may be passed to munmap(2) with a
> > > + * length of `(size_t)num * (size_t)size_per_each`, in order to deallocate the
> > > + * memory, after which it is invalid to pass it to vDSO getrandom().
> >
> > Wouldn't a munmap with '(size_t)num * (size_t)size_per_each' be potentially too
> > short, due to how the allocation is sized such that states don't cross page
> > boundaries?
>
> You're right, I think. The calculation should instead be something like:
>
> DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE
>
> Does that seem correct to you?
>
Yes, though I wonder if it would be better to give userspace the number of pages
instead of the number of states.
- Eric
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christoph Hellwig @ 2024-06-04 5:22 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Miklos Szeredi, Christoph Hellwig, Christian Brauner, Jan Kara,
Alexander Viro, Chuck Lever, Jeff Layton, Amir Goldstein,
Alexander Aring, linux-fsdevel, linux-nfs, linux-kernel,
linux-api
In-Reply-To: <20240529.013815-fishy.value.nervous.brutes-FzobWXrzoo2@cyphar.com>
On Sat, Jun 01, 2024 at 01:12:31AM -0700, Aleksa Sarai wrote:
> Not to mention that providing a mount fd is what allows for extensions
> like Christian's proposed method of allowing restricted forms of
> open_by_handle_at() to be used by unprivileged users.
As mentioned there I find the concept of an unprivileged
open_by_handle_at extremely questionable as it trivially gives access to
any inode on the file systems.
> If file handles really are going to end up being the "correct" mechanism
> of referencing inodes by userspace,
They aren't.
> then future API designs really need
> to stop assuming that the user is capable(CAP_DAC_READ_SEARCH).
There is no way to support open by handle for unprivileged users. The
concept of an inode number based file handle simply does not work for
that at all.
^ permalink raw reply
* Re: Header conflicts with shmget() and SHM_HUGE_2MB
From: Andi Kleen @ 2024-06-03 19:27 UTC (permalink / raw)
To: Carlos Llamas
Cc: linux-mm, Mike Kravetz, Hugh Dickins, Edward Liaw, kernel-team,
linux-kernel, linux-api
In-Reply-To: <Zl4LC9lTNptB2xTJ@google.com>
> I can see such definitions are tagged as "obsolete" in the uapi headers.
> Do we need some ifndef protection with the glibc headers?
They should be still supported, but also the more generic macro.
>
> What is the advice to follow for userspace? Skip <linux/shm.h> and
> openly redefine the SHM_HUGE_* wherever needed?
glibc (or other C libraries if not using linux/shm) should add the
defines. Short term you would need to redefine on your own yes.
-Andi
^ permalink raw reply
* Header conflicts with shmget() and SHM_HUGE_2MB
From: Carlos Llamas @ 2024-06-03 18:27 UTC (permalink / raw)
To: linux-mm
Cc: Mike Kravetz, Andi Kleen, Hugh Dickins, Edward Liaw, kernel-team,
linux-kernel, linux-api
Hi, I'm trying to figure out how one would use SHM_HUGE_{2MB/1GB}
defines through shmget() from userspace. After having a look at the
man-pages I thought the #include pattern would be similar to that of
mmap(), e.g.:
#include <sys/mman.h>
#include <linux/mman.h>
[...]
mmap(NULL, size, prot, flags | MAP_HUGETLB | MAP_HUGE_2MB,
fd, 0);
However, when doing the shmem equivalent with the headers I get several
redefinition conflicts. When attempting to compile something like this:
#include <sys/shm.h>
#include <linux/shm.h>
[...]
shmid = shmget(key, size, flags | SHM_HUGETLB | SHM_HUGE_2MB);
I run into the following type of issues:
/usr/include/linux/shm.h:26:8: error: redefinition of ‘struct shmid_ds’
26 | struct shmid_ds {
| ^~~~~~~~
In file included from /usr/include/x86_64-linux-gnu/bits/shm.h:45,
from /usr/include/x86_64-linux-gnu/sys/shm.h:30:
/usr/include/x86_64-linux-gnu/bits/types/struct_shmid_ds.h:24:8: note: originally defined here
24 | struct shmid_ds
| ^~~~~~~~
I can see such definitions are tagged as "obsolete" in the uapi headers.
Do we need some ifndef protection with the glibc headers?
What is the advice to follow for userspace? Skip <linux/shm.h> and
openly redefine the SHM_HUGE_* wherever needed?
Thanks,
--
Carlos Llamas
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Jan Kara @ 2024-06-03 10:30 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Miklos Szeredi, Christoph Hellwig, Christian Brauner, Jan Kara,
Alexander Viro, Chuck Lever, Jeff Layton, Amir Goldstein,
Alexander Aring, linux-fsdevel, linux-nfs, linux-kernel,
linux-api
In-Reply-To: <20240529.013815-fishy.value.nervous.brutes-FzobWXrzoo2@cyphar.com>
On Sat 01-06-24 01:12:31, Aleksa Sarai wrote:
> On 2024-05-28, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Tue, 28 May 2024 at 15:24, Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> > > > Can you please explain how opening an fd based on a handle returned from
> > > > name_to_handle_at() and not using a mount file descriptor for
> > > > open_by_handle_at() would work?
> > >
> > > Same as NFS file handles:
> > >
> > > name_to_handle_at returns a handle that includes a file system
> > > identifier.
> > >
> > > open_by_handle_at looks up the superblock based on that identifier.
> >
> > The open file needs a specific mount, holding the superblock is not sufficient.
>
> Not to mention that providing a mount fd is what allows for extensions
> like Christian's proposed method of allowing restricted forms of
> open_by_handle_at() to be used by unprivileged users.
>
> If file handles really are going to end up being the "correct" mechanism
> of referencing inodes by userspace, then future API designs really need
> to stop assuming that the user is capable(CAP_DAC_READ_SEARCH). Being
> able to open any file in any superblock the kernel knows about
> (presumably using a kernel-internal mount if we are getting rid of the
> mount fd) is also capable(CAP_SYS_ADMIN) territory.
Well, but this is already handled - name_to_handle_at() with AT_HANDLE_FID
is completely unpriviledged operation. Unpriviledged userspace can use
fhandle for comparisons with other file handles but that's all it is good
for (similarly as inode number you get from statx(2) but does not have the
problem with inode number uniqueness on btrfs, bcachefs, etc.). I don't
expect unpriviledged userspace to be able to more with the fhandle it got.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v16 3/5] arch: allocate vgetrandom_alloc() syscall number
From: Jason A. Donenfeld @ 2024-06-01 10:58 UTC (permalink / raw)
To: Eric Biggers
Cc: Geert Uytterhoeven, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand
In-Reply-To: <20240531022621.GC1502@sol.localdomain>
On Thu, May 30, 2024 at 07:26:21PM -0700, Eric Biggers wrote:
> On Tue, May 28, 2024 at 03:28:58PM +0200, Jason A. Donenfeld wrote:
> > On Tue, May 28, 2024 at 3:10 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > On Tue, May 28, 2024 at 03:08:00PM +0200, Geert Uytterhoeven wrote:
> > > > Hi Jason,
> > > >
> > > > On Tue, May 28, 2024 at 2:24 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > > Add vgetrandom_alloc() as syscall 462 (or 572 on alpha) by adding it to
> > > > > all of the various syscall.tbl and unistd.h files.
> > > > >
> > > > > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > >
> > > > As of commit ff388fe5c481d39c ("mseal: wire up mseal syscall") in
> > > > v6.10-rc1, 462 is already taken.
> > > >
> > > > v17 ++ ;-)
> > >
> > > Oy! Thanks. I should have thought to rebase on rc1 anyway before posting
> > > this.
> >
> > Sorted in https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=vdso
> > for the time being.
> >
>
> Please also get in the habit of using the --base option to git format-patch, so
> that it's actually possible to apply patches without guessing the base commit.
I recall you mentioning this to me in the past. I'll experiment with it
for the v+1.
Jason
^ permalink raw reply
* Re: [PATCH v16 2/5] random: add vgetrandom_alloc() syscall
From: Jason A. Donenfeld @ 2024-06-01 10:56 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240531035917.GD6505@sol.localdomain>
On Thu, May 30, 2024 at 08:59:17PM -0700, Eric Biggers wrote:
> On Tue, May 28, 2024 at 02:19:51PM +0200, Jason A. Donenfeld wrote:
> > +/**
> > + * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
> > + *
> > + * @num: On input, a pointer to a suggested hint of how many states to
> > + * allocate, and on return the number of states actually allocated.
> > + *
> > + * @size_per_each: On input, must be zero. On return, the size of each state allocated,
> > + * so that the caller can split up the returned allocation into
> > + * individual states.
> > + *
> > + * @addr: Reserved, must be zero.
> > + *
> > + * @flags: Reserved, must be zero.
> > + *
> > + * The getrandom() vDSO function in userspace requires an opaque state, which
> > + * this function allocates by mapping a certain number of special pages into
> > + * the calling process. It takes a hint as to the number of opaque states
> > + * desired, and provides the caller with the number of opaque states actually
> > + * allocated, the size of each one in bytes, and the address of the first
> > + * state, which may be split up into @num states of @size_per_each bytes each,
> > + * by adding @size_per_each to the returned first state @num times, while
> > + * ensuring that no single state straddles a page boundary.
> > + *
> > + * Returns the address of the first state in the allocation on success, or a
> > + * negative error value on failure.
> > + *
> > + * The returned address of the first state may be passed to munmap(2) with a
> > + * length of `(size_t)num * (size_t)size_per_each`, in order to deallocate the
> > + * memory, after which it is invalid to pass it to vDSO getrandom().
>
> Wouldn't a munmap with '(size_t)num * (size_t)size_per_each' be potentially too
> short, due to how the allocation is sized such that states don't cross page
> boundaries?
You're right, I think. The calculation should instead be something like:
DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE
Does that seem correct to you?
Jason
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-06-01 8:12 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Christoph Hellwig, Christian Brauner, Jan Kara, Alexander Viro,
Chuck Lever, Jeff Layton, Amir Goldstein, Alexander Aring,
linux-fsdevel, linux-nfs, linux-kernel, linux-api
In-Reply-To: <CAJfpegvznUGTYxxTzB5QQHWtNrCfSkWvGscacfZ67Gn+6XoD8w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]
On 2024-05-28, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, 28 May 2024 at 15:24, Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> > > Can you please explain how opening an fd based on a handle returned from
> > > name_to_handle_at() and not using a mount file descriptor for
> > > open_by_handle_at() would work?
> >
> > Same as NFS file handles:
> >
> > name_to_handle_at returns a handle that includes a file system
> > identifier.
> >
> > open_by_handle_at looks up the superblock based on that identifier.
>
> The open file needs a specific mount, holding the superblock is not sufficient.
Not to mention that providing a mount fd is what allows for extensions
like Christian's proposed method of allowing restricted forms of
open_by_handle_at() to be used by unprivileged users.
If file handles really are going to end up being the "correct" mechanism
of referencing inodes by userspace, then future API designs really need
to stop assuming that the user is capable(CAP_DAC_READ_SEARCH). Being
able to open any file in any superblock the kernel knows about
(presumably using a kernel-internal mount if we are getting rid of the
mount fd) is also capable(CAP_SYS_ADMIN) territory.
Would the idea be to sign or MAC every file handle to avoid userspace
being able to brute-force the file handle of anything the system sees?
What happens if the key has to change? Then the handles aren't globally
unique anymore...
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation
From: Andy Lutomirski @ 2024-05-31 23:06 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240528122352.2485958-5-Jason@zx2c4.com>
> On May 28, 2024, at 5:25 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Provide a generic C vDSO getrandom() implementation, which operates on
> an opaque state returned by vgetrandom_alloc() and produces random bytes
> the same way as getrandom(). This has a the API signature:
>
> ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state);
> +/**
> + * type vdso_kernel_ulong - unsigned long type that matches kernel's unsigned long
> + *
> + * Data shared between userspace and the kernel must operate the same way in both 64-bit code and in
> + * 32-bit compat code, over the same potentially 64-bit kernel. This type represents the size of an
> + * unsigned long as used by kernel code. This isn't necessarily the same as an unsigned long as used
> + * by userspace, however.
Why is this better than using plain u64? It’s certainly more
complicated. It also rather fundamentally breaks CRIU on 32-bit
userspace (although CRIU may well be unable to keep vgetrandom working
after a restore onto a different kernel anyway). Admittedly 32-bit
userspace is a slowly dying breed, but still.
> + *
> + * +-------------------+-------------------+------------------+-------------------+
> + * | 32-bit userspace | 32-bit userspace | 64-bit userspace | 64-bit userspace |
> + * | unsigned long | vdso_kernel_ulong | unsigned long | vdso_kernel_ulong |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
> + * | 32-bit kernel | ✓ same size | ✓ same size |
> + * | unsigned long | | |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
> + * | 64-bit kernel | ✘ different size! | ✓ same size | ✓ same size | ✓ same size |
> + * | unsigned long | | | | |
> + * +---------------+-------------------+-------------------+------------------+-------------------+
> + */
> +#ifdef CONFIG_64BIT
> +typedef u64 vdso_kernel_ulong;
> +#else
> +typedef u32 vdso_kernel_ulong;
> +#endif
> +
> +#endif /* __VDSO_TYPES_H */
> diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
> new file mode 100644
> index 000000000000..4d9bb59985f8
> --- /dev/null
> +++ b/lib/vdso/getrandom.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +#include <linux/cache.h>
> +#include <linux/kernel.h>
> +#include <linux/time64.h>
> +#include <vdso/datapage.h>
> +#include <vdso/getrandom.h>
> +#include <asm/vdso/getrandom.h>
> +#include <asm/vdso/vsyscall.h>
> +
> +#define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do { \
> + while (len >= sizeof(type)) { \
> + __put_unaligned_t(type, __get_unaligned_t(type, src), dst); \
> + __put_unaligned_t(type, 0, src); \
> + dst += sizeof(type); \
> + src += sizeof(type); \
> + len -= sizeof(type); \
> + } \
> +} while (0)
> +
> +static void memcpy_and_zero_src(void *dst, void *src, size_t len)
> +{
> + if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
> + if (IS_ENABLED(CONFIG_64BIT))
> + MEMCPY_AND_ZERO_SRC(u64, dst, src, len);
> + MEMCPY_AND_ZERO_SRC(u32, dst, src, len);
> + MEMCPY_AND_ZERO_SRC(u16, dst, src, len);
> + }
> + MEMCPY_AND_ZERO_SRC(u8, dst, src, len);
> +}
> +
> +/**
> + * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
> + * @rng_info: Describes state of kernel RNG, memory shared with kernel.
> + * @buffer: Destination buffer to fill with random bytes.
> + * @len: Size of @buffer in bytes.
> + * @flags: Zero or more GRND_* flags.
> + * @opaque_state: Pointer to an opaque state area.
> + *
> + * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
> + * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
> + * schedule that the kernel's RNG is reseeded. If the kernel's RNG is not ready, then this always
> + * calls into the syscall.
> + *
> + * @opaque_state *must* be allocated using the vgetrandom_alloc() syscall. Unless external locking
> + * is used, one state must be allocated per thread, as it is not safe to call this function
> + * concurrently with the same @opaque_state. However, it is safe to call this using the same
> + * @opaque_state that is shared between main code and signal handling code, within the same thread.
> + *
> + * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
> + */
> +static __always_inline ssize_t
> +__cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
> + unsigned int flags, void *opaque_state)
I don’t love this function signature. I generally think that, if
you’re going to have user code pass a pointer to kernel code, either
make the buffer have a well defined, constant size or pass a length.
As it stands, one cannot locally prove that user code that calls it is
memory-safe. In fact, any caller that has the misfortune of running
under CRIU is *not* memory safe if CRIU allows the vDSO to be
preserved. Ouch. (CRIU has some special code for this. I'm not 100%
clear on all the details.) One could maybe sort of get away with
treating the provided opaque_state as a completely opaque value and
not a pointer, but then the mechanism for allocating these states
should be adjusted accordingly.
One thing that occurs to me is that, if this thing were to be made
CRIU-safe, the buffer could have a magic number that changes any time
the data structure changes, and the vDSO could check, at the beginning
and end of the call, that the magic number is correct. Doing this
would require using a special VMA type instead of just a wipe-on-fork
mapping, which could plausibly be a good thing anyway. (Hmm, we don't
just want WIPEONFORK. We should probably also wipe on swap-out and,
more importantly, we should absolutely wipe on any sort of CRIU-style
checkpointing. Perhaps a special VMA would be a good thing for
multiple reasons.
> +{
> + ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
> + struct vgetrandom_state *state = opaque_state;
> + size_t batch_len, nblocks, orig_len = len;
> + unsigned long current_generation;
> + void *orig_buffer = buffer;
> + u32 counter[2] = { 0 };
> + bool in_use, have_retried = false;
> +
> + /* The state must not straddle a page, since pages can be zeroed at any time. */
> + if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
> + goto fallback_syscall;
This is weird. Either the provided pointer is valid or it isn’t.
Reasonable outcomes are a segfault if the pointer is bad or success
(or fallback if needed for some reason) if the pointer is good. Why
is there specific code to catch a specific sort of pointer screwup
here?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox