Re: [PATCH v11 1/4] random: add vgetrandom_alloc() syscall

[Florian Weimer <fweimer@redhat.com>]

* Jason A. Donenfeld:

> Hi Jann,
>
> On Mon, Dec 05, 2022 at 08:13:36PM +0100, Jann Horn wrote:
>> On Mon, Dec 5, 2022 at 3:01 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > +       mm->def_flags |=
>> > +               /*
>> > +                * Don't allow state to be written to swap, to preserve forward secrecy.
>> > +                * This works in conjunction with MAP_LOCKED in do_mmap(), below, which
>> > +                * actually does the locking (and associated permission check and accounting).
>> > +                * Here, VM_LOCKONFAULT together with VM_NORESERVE simply make the mlocking
>> > +                * happen the first time it's actually used, the same as when calling
>> > +                * mlock2(MLOCK_ONFAULT) from userspace.
>> > +                */
>> > +               VM_LOCKONFAULT | VM_NORESERVE |
>> 
>> Have you checked the interaction with this line in dup_mmap()?
>> "tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT);"
>> 
>> As the mlock.2 manpage says, "Memory locks are not inherited by a
>> child created via fork(2)". I think the intention here is that the VMA
>> should stay unswappable after fork(), right?
>> 
>> Of course, trying to reserve more mlocked memory in fork() would also
>> be problematic...
>
> Thanks for pointing that out! Indeed that seems problematic.
> Fortunately, the use of WIPEONFORK at the same time as LOCKONFAULT means
> that memory doesn't actually need to be reserved in fork() itself. So
> something like the below seems correct and doable.
>
> Jason
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ec57cae58ff1..cd53ffff615d 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -656,7 +656,9 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>  			tmp->anon_vma = NULL;
>  		} else if (anon_vma_fork(tmp, mpnt))
>  			goto fail_nomem_anon_vma_fork;
> -		tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT);
> +		if ((tmp->vm_flags & (VM_LOCKONFAULT | VM_WIPEONFORK)) !=
> +		    (VM_LOCKONFAULT | VM_WIPEONFORK))
> +			tmp->vm_flags &= ~(VM_LOCKED | VM_LOCKONFAULT);
>  		file = tmp->vm_file;
>  		if (file) {
>  			struct address_space *mapping = file->f_mapping;

Still it's a bit concerning that calling getrandom (the libc function)
now apparently can kill the process if the system is under severe memory
pressure.  In many cases, that's okay, but we wouldn't want that for
PID 1, for example.  vm.overcommit_memory=2 mode is supposed to prevent
such crashes, and I think NORESERVE (not shown here) sidesteps that.

Thanks,
Florian