linux-kernel - Re: [PATCH v11 1/4] random: add vgetrandom

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y45OWhyN+U975vIN@zx2c4.com>
Date:   Mon, 5 Dec 2022 21:02:34 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     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>,
        Florian Weimer <fweimer@...hat.com>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v11 1/4] random: add vgetrandom_alloc() syscall

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;