Message-ID: <Y488SnSlQALH9+Yk@zx2c4.com>
Date:   Tue, 6 Dec 2022 13:57:46 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Florian Weimer <fweimer@...hat.com>
Cc:     Jann Horn <jannh@...gle.com>, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev, tglx@...utronix.de,
        linux-crypto@...r.kernel.org, linux-api@...r.kernel.org,
        x86@...nel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>,
        Carlos O'Donell <carlos@...hat.com>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Brauner <brauner@...nel.org>, linux-mm@...ck.org,
        Liam.Howlett@...cle.com, akpm@...ux-foundation.org, vbabka@...e.cz
Subject: Re: [PATCH v11 1/4] random: add vgetrandom_alloc() syscall

Hi Florian,

On Mon, Dec 05, 2022 at 09:06:06PM +0100, Florian Weimer wrote:
> * 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@...c4.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.

Right. Setting VM_NORESERVE this way sidesteps it. Passing MAP_NORESERVE
to do_mmap() would make it respect vm.overcommit_memory=2, but then we'd
face problems at fork() time, as Jann pointed out, when we might go down
the path of trying to mlock memory from the fork() handler, and that
seems not so desirable. But moreover, the overcommitment potentially
makes the allocation scheme a lot simpler for libcs. Do any init daemons
actually use vm.overcommit_memory=2? Is this actually something to care
about?

If this isn't something we really care about so much, then my little
diff above should suffice, and this all remains very simple. I suspect
that's the case, because there are several VMAs that get set with
VM_NORESERVE already in the kernel.

If this is something we need to care about, then perhaps it's worth
rethinking the problem space from its basic goals:

- This memory must not be written to swap. Even if we added some flag to
  zero that part of swap when paging back in, that wouldn't cut it,
  because it's often very hard to "truly" zero out disk writes (and
  nobody wants to TRIM so often). (Rationale: forward secrecy.)

- It would be "nice" if untouched allocations didn't actually take
  up any memory.

- This needs to be wiped on fork.

Complications thus far encountered:

- VM_LOCKED|VM_LOCKONFAULT isn't inherited by forks.

- You're worried about forcing VM_LOCKONFAULT (rightfully or wrongly,
  as yet established).

However, there are two useful characteristics of this series that we
might be able to exploit in order to arrive at a solution:

1) Due to being wiped during fork(), the code is already robust to
   having the contents of those pages zeroed out midway through.

2) In the absolute worst case of whatever contingency we're coding for,
   we have the option to fallback to the getrandom() syscall, and
   everything is fine.

So, putting together the basic goals with the complications thus far
encountered, and trying to make use of (1) and (2), what if we introduce
a VM_DROPPABLE flag. The semantics would be:

a) It never is written out to swap.
b) No memory is pre-reserved / committed.
c) Under memory pressure, mm can just drop the pages and make them zero.
d) If there's not enough memory to service a page fault, it's not fatal,
   and no signal is sent. Instead, writes are simply lost, and reads
   return zero, as if the page was dropped.
e) It is inherited by fork.
f) The pages are zeroed on fork (like VM_WIPEONFORK).
g) It doesn't count against the mlock budget, since nothing is locked.

Then, as an emergent restriction, we require that each opaque_state
never straddle two pages, by returning a rounded up size_per_each.

What do you think of this plan? It's harder to implement, so I still
would prefer the simpler diff I sent to Jann above. But if you're really
convinced that disrespecting vm.overcommit_memory=2 is abominable, then
maybe this more complicated plan could work. Plus, semantic (g) has its
own advantages alone.

I'm CC'ing linux-mm about this matter, as I'm sure they'll have
something to contribute here. (And linux-mm@, if your reaction is "why
do we need this syscall at all, can't userspace just bla bla bla bla",
please read the cover letter of the series, this patch's commit message,
and prior discussion on that topic, so we don't have to rehash that.)

Jason

Powered by blists - more mailing lists