Re: [RFC PATCH v2 5/8] mseal: Check seal flag for munmap(2)

[Jeff Xu]

On Tue, Oct 17, 2023 at 9:54 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 17 Oct 2023 at 02:08, <jeffxu@xxxxxxxxxxxx> wrote:
> >
> > Of all the call paths that call into do_vmi_munmap(),
> > this is the only place where checkSeals = MM_SEAL_MUNMAP.
> > The rest has checkSeals = 0.
>
> Why?
>
> None of this makes sense.
>
> So you say "we can't munmap in this *one* place, but all others ignore
> the sealing".
>
I apologize that previously, I described what this code does, and not reasoning.

In our threat model, as Stephen Röttger point out in [1], and I quote:

V8 exploits typically follow a similar pattern: an initial bug leads
to memory corruption but often the initial corruption is limited and
the attacker has to find a way to arbitrarily read/write in the whole
address space.

The memory correction is in the user space process, e.g. Chrome.
Attackers will try to modify permission of the memory, by calling
mprotect,  or munmap then mmap to the same address but with different
permission, etc.

Sealing blocks mprotect/munmap/mremap/mmap call from the user space
process, e.g. Chrome.

At time of handling those 4 syscalls, we need to check the seal (
can_modify_mm), this requires locking the VMA (
mmap_write_lock_killable), and ideally, after validating the syscall
input. The reasonable place for can_modify_mm() is from utility
functions, such as do_mmap(), do_vmi_munmap(), etc.

However, there is no guarantee that do_mmap() and do_vmi_munmap() are
only reachable from mprotect/munmap/mremap/mmap syscall entry point
(SYSCALL_DEFINE_XX). In theory,  the kernel can call those in other
scenarios, and some of them can be perfectly legit. Those other
scenarios are not covered by our threat model at this time. Therefore,
we need a flag, passed from the SYSCALL_DEFINE_XX entry , down to
can_modify_mm(), to differentiate those other scenarios.

Now, back to code, it did some optimization, i.e. doesn't pass the
flag from SYSCALL_DEFINE_XX  in all cases. If SYSCALL_DEFINE_XX calls
do_a, and do_a has only one caller, I will set the flag in do_a,
instead of SYSCALL_DEFINE_XX. Doing this reduces the size of the
patchset, but it also makes the code less readable indeed. I could
remove this optimization in V3. I welcome suggestions to improve
readability on this.

When handing the mmap/munmap/mremap/mmap, once the code passed
can_modify_mm(), it means the memory area is not sealed, if the code
continues to call the other utility functions, we don't need to check
the seal again. This is the case for mremap(), the seal of src address
and dest address (when applicable) are checked first, later when the
code calls  do_vmi_munmap(), it no longer needs to check the seal
again.

[1] https://v8.dev/blog/control-flow-integrity

-Jeff