Message-ID: <uwwg47m4mwo3g32qavzr2mjmh4r6lcm3irr3wtlvedlylbq74z@flcq2kwvmh46>
Date: Fri, 4 Oct 2024 18:02:24 +0100
From: Pedro Falcato <pedro.falcato@...il.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: akpm@...ux-foundation.org, keescook@...omium.org, corbet@....net, 
	jorgelo@...omium.org, groeck@...omium.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org, jannh@...gle.com, sroettger@...gle.com, 
	linux-hardening@...r.kernel.org, willy@...radead.org, gregkh@...uxfoundation.org, 
	torvalds@...ux-foundation.org, deraadt@...nbsd.org, usama.anjum@...labora.com, 
	surenb@...gle.com, merimus@...gle.com, rdunlap@...radead.org, 
	lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, enh@...gle.com
Subject: Re: [PATCH v1 1/1] mseal: update mseal.rst

On Mon, Sep 30, 2024 at 05:24:39PM -0700, Jeff Xu wrote:
> Hi Pedro
> 
> On Sat, Sep 28, 2024 at 6:43 AM Pedro Falcato <pedro.falcato@...il.com> wrote:
> >
> > On Fri, Sep 27, 2024 at 06:29:30PM GMT, Jeff Xu wrote:
> > > Hi Pedro,
> > >
> > > On Fri, Sep 27, 2024 at 3:59 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
> > <snip>
> > > > > +
> > > > > +   Blocked mm syscall:
> > > > > +      - munmap
> > > > > +      - mmap
> > > > > +      - mremap
> > > > > +      - mprotect and pkey_mprotect
> > > > > +      - some destructive madvise behaviors: MADV_DONTNEED, MADV_FREE,
> > > > > +        MADV_DONTNEED_LOCKED, MADV_FREE, MADV_DONTFORK, MADV_WIPEONFORK
> > > > > +
> > > > > +   The first set of syscall to block is munmap, mremap, mmap. They can
> > > > > +   either leave an empty space in the address space, therefore allow
> > > > > +   replacement with a new mapping with new set of attributes, or can
> > > > > +   overwrite the existing mapping with another mapping.
> > > > > +
> > > > > +   mprotect and pkey_mprotect are blocked because they changes the
> > > >                                                           change
> > > > > +   protection bits (rwx) of the mapping.
> > > > > +
> > > > > +   Some destructive madvice behaviors (MADV_DONTNEED, MADV_FREE,
> > > > > +   MADV_DONTNEED_LOCKED, MADV_FREE, MADV_DONTFORK, MADV_WIPEONFORK)
> > > > > +   for anonymous memory, when users don't have write permission to the
> > > > > +   memory. Those behaviors can alter region contents by discarding pages,
> > > > > +   effectively a memset(0) for anonymous memory.
> > > >
> > > > What's the difference between anonymous memory and MAP_PRIVATE | MAP_FILE?
> > > >
> > > MAP_FILE seems not used ?
> > > anonymous mapping is the mapping that is not backed by a file.
> >
> > MAP_FILE is actually defined as 0 usually :) But I meant file-backed private mappings.
> >
> OK, we are on the same page for this.
> 
> > > > The feature now, as is (as far as I understand!) will allow you to do things like MADV_DONTNEED
> > > > on a read-only file mapping. e.g .text. This is obviously wrong?
> > > >
> > > When a MADV_DONTNEED is called, pages will be freed, on file-backed
> > > mapping,  if the process reads from the mapping again, the content
> > > will be retrieved from the file.
> > >
> >
> > Sorry, it was late and I gave you a crap example. Consider this:
> > a file-backed MAP_PRIVATE vma is marked RW. I write to it, then RO-it + mseal.
> >
> > The attacker later gets me to MADV_DONTNEED that VMA. You've just lost data.
> >
> > The big problem here is with anon _pages_, not anon vmas.
> >
> That depends on the app's threat-model. What you described seems to be
> a case below
> 1. The file is rw
> 2. The process opens the file as rw
> 3. the process mmap the fd as rw
> 4 The process writes the memory, and the change isn't flushed to the
> file on disk.
> 5 The process  changes the mapping to RO
> 6. The process seals the mapping
> 7. The process is called MADV_DONTNEED , and because the change isn't
> flush to file on disk, so it loses the change, (retrieve the old data
> from disk when read from the mapped address later)
> 
> I'm not sure this is a valid use case, the problem here seems to be
> that the app needs to flush the change from memory to disk if the
> expectation is writing is permanent.
>

MAP_PRIVATE never does writeback. That's not what this is about.
I can trivially discard anonymous pages for private "file VMAs", which aren't
refilled with the exact same contents. This is a problem.

> In any case, the mseal currently just blocks a subset of madvise, those
> we know with a security implication.  If there is something mseal needs
> to block additionally, one can always extend it by using the "flags" field.
> I do think the bar is high though, e.g. a valid use case to support that.

No, this has nothing to do with a flag. It's about providing sane semantics.

> 
> > > For anonymous mapping, since  there is no file backup, if process
> > > reads from the mapping, 0 is filled, hence equivalent to memset(0)
> > >
> > > > > +
> > > > > +   Kernel will return -EPERM for blocked syscalls.
> > > > > +
> > > > > +   When blocked syscall return -EPERM due to sealing, the memory regions may or may not be changed, depends on the syscall being blocked:
> > > > > +      - munmap: munmap is atomic. If one of VMAs in the given range is
> > > > > +        sealed, none of VMAs are updated.
> > > > > +      - mprotect, pkey_mprotect, madvise: partial update might happen, e.g.
> > > > > +        when mprotect over multiple VMAs, mprotect might update the beginning
> > > > > +        VMAs before reaching the sealed VMA and return -EPERM.
> > > > > +      - mmap and mremap: undefined behavior.
> > > >
> > > > mmap and mremap are actually not undefined as they use munmap semantics for their unmapping.
> > > > Whether this is something we'd want to document, I don't know honestly (nor do I think is ever written down in POSIX?)
> > > >
> > > I'm not sure if I can declare mmap/mremap as atomic.
> > >
> > > Although, it might be possible to achieve this due to munmap being
> > > atomic. I'm not sure  as I didn't test this. Would you like to find
> > > out ?
> >
> > I just told you they use munmap under the hood. It's just that the requirement isn't actually
> > written down anywhere.
> >
> I knew about mmap/mremap calling munmap. I don't know what exactly you
> are asking though. In your patch and its discussion, you did not mention
> the mmap/mremap (for sealing) is or should be atomic.
> 
> My point is: since there isn't a clear statement from your patch description
> or POSIX, that mremap/mmap is atomic,  and I haven't tested it myself with
> regards to sealing, let's  leave them as "undefined" for now. (I could get back
> to this later after the merging window)
> 
> > >
> > > > >
> > > > >  Use cases:
> > > > >  ==========
> > > > >  - glibc:
> > > > >    The dynamic linker, during loading ELF executables, can apply sealing to
> > > > > -  non-writable memory segments.
> > > > > +  mapping segments.
> > > > >
> > > > >  - Chrome browser: protect some security sensitive data-structures.
> > > > >
> > > > > -Notes on which memory to seal:
> > > > > -==============================
> > > > > -
> > > > > -It might be important to note that sealing changes the lifetime of a mapping,
> > > > > -i.e. the sealed mapping won’t be unmapped till the process terminates or the
> > > > > -exec system call is invoked. Applications can apply sealing to any virtual
> > > > > -memory region from userspace, but it is crucial to thoroughly analyze the
> > > > > -mapping's lifetime prior to apply the sealing.
> > > > > +Don't use mseal on:
> > > > > +===================
> > > > > +Applications can apply sealing to any virtual memory region from userspace,
> > > > > +but it is *crucial to thoroughly analyze the mapping's lifetime* prior to
> > > > > +apply the sealing. This is because the sealed mapping *won’t be unmapped*
> > > > > +till the process terminates or the exec system call is invoked.
> > > >
> > > > There should probably be a nice disclaimer as to how most people don't need this or shouldn't use this.
> > > > At least in its current form.
> > > >
> > > Ya, the mseal is not for most apps. I mention the malloc example to stress that.
> > >
> > > > <snip>
> > > > > -
> > > > > -
> > > > > -Additional notes:
> > > > > -=================
> > > > >  As Jann Horn pointed out in [3], there are still a few ways to write
> > > > > -to RO memory, which is, in a way, by design. Those cases are not covered
> > > > > -by mseal(). If applications want to block such cases, sandbox tools (such as
> > > > > -seccomp, LSM, etc) might be considered.
> > > > > +to RO memory, which is, in a way, by design. And those could be blocked
> > > > > +by different security measures.
> > > > >
> > > > >  Those cases are:
> > > > > -
> > > > > -- Write to read-only memory through /proc/self/mem interface.
> > > > > -- Write to read-only memory through ptrace (such as PTRACE_POKETEXT).
> > > > > -- userfaultfd.
> > > > > +   - Write to read-only memory through /proc/self/mem interface (FOLL_FORCE).
> > > > > +   - Write to read-only memory through ptrace (such as PTRACE_POKETEXT).
> > > > > +   - userfaultfd.
> > > >
> > > > I don't understand how this is not a problem, but MADV_DONTNEED is.
> > > > To me it seems that what we have now is completely useless, because you can trivially
> > > > bypass it using /proc/self/mem, which is enabled on most Linux systems.
> > > >
> > > > Before you mention ChromeOS or Chrome, I don't care. Kernel features aren't designed
> > > > for Chrome. They need to work with every other distro and application as well.
> > > >
> > > > It seems to me that the most sensible change is blocking/somehow distinguishing between /proc/self/mem and
> > > > /proc/<pid>/mem (some other process) and ptrace. As in blocking /proc/self/mem but allowing the other FOLL_FORCE's
> > > > as the traditional UNIX permission model allows.
> > > >
> > > IMO, it is a matter of  Divide and Conquer.  In a nutshell, mseal only
> > > prevents VMA's certain attributes (such as prot bits) from changing.
> > > It doesn't mean to say that sealed RO memory is immutable. To achieve
> > > that, the system needs to apply multiple security measures.
> >
> > No, it's a matter of providing a sane API without tons of edgecases. Making a VMA immutable should make a VMA
> > immutable, and not require you to provide a crap ton of other mechanisms in order to truly make it immutable.
> > If I call mseal, I expect it to be sealed, not "sealed except when it's not, lol".
> >
> > You haven't been able to quite specify what semantics are desirable out of this whole thing. Making
> > prot flags "immutable" is completely worthless if you can simply write to a random pseudofile and
> > have it bypass the whole thing (where a write to /proc/self/mem is semantically equivalent to
> > mprotect RW + write + mprotect RO). Making the vma immutable is completely worthless
> > if I can simply wipe anon pages. There has to be some end goal here (make contents immutable?
> > make sure VMA protection can't be changed? both?) which seems to be unclear from the kernel mmap-side.
> >
> > If you insist on providing half-baked APIs (and waving off any concerns), I'm sure this would've been better
> > implemented as a random bpf program for chrome. Maybe we could revert this whole thing and give eBPF one
> > or two bits of vma flags for their own uses :)
> >

Please reply to the above. We're struggling to understand exactly what semantics you want from this.
*That* is what we want to document and get set in stone, and we'll move from there.

> > >
> > > For writing to /proc/pid/mem, it can be disabled via [1].  SELINUX and
> > > Landlock can achieve the same protection too.
> >
> > I'm not blocking /proc/pid/mem, and my distro doesn't run any of those security modules :/
> >
> It is a choice you can make :-)

Your feature needs to work without "extra choices".

-- 
Pedro

Powered by blists - more mailing lists