Message-ID: <CALmYWFtTDAb_kpZdAe_xspqwNgK1NWJmjTxaTC=jDEMzfe297Q@mail.gmail.com>
Date:   Tue, 17 Oct 2023 02:07:52 -0700
From:   Jeff Xu <jeffxu@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     jeffxu@...omium.org, akpm@...ux-foundation.org,
        keescook@...omium.org, sroettger@...gle.com, jorgelo@...omium.org,
        groeck@...omium.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
        jannh@...gle.com, surenb@...gle.com, alex.sierra@....com,
        apopple@...dia.com, aneesh.kumar@...ux.ibm.com,
        axelrasmussen@...gle.com, ben@...adent.org.uk,
        catalin.marinas@....com, david@...hat.com, dwmw@...zon.co.uk,
        ying.huang@...el.com, hughd@...gle.com, joey.gouly@....com,
        corbet@....net, wangkefeng.wang@...wei.com,
        Liam.Howlett@...cle.com, lstoakes@...il.com, willy@...radead.org,
        mawupeng1@...wei.com, linmiaohe@...wei.com, namit@...are.com,
        peterx@...hat.com, peterz@...radead.org, ryan.roberts@....com,
        shr@...kernel.io, vbabka@...e.cz, xiujianfeng@...wei.com,
        yu.ma@...el.com, zhangpeng362@...wei.com, dave.hansen@...el.com,
        luto@...nel.org, linux-hardening@...r.kernel.org
Subject: Re: [RFC PATCH v1 0/8] Introduce mseal() syscall

Hello Linus,

Thank you for the time reviewing this patch set.

On Mon, Oct 16, 2023 at 10:23 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Mon, 16 Oct 2023 at 07:38, <jeffxu@...omium.org> wrote:
> >
> > This patchset proposes a new mseal() syscall for the Linux kernel.
>
> So I have no objections to adding some kind of "lock down memory
> mappings" model, but this isn't it.
>
> First off, the simple stuff: the commit messages are worthless. Having
>
>    check seal for mmap(2)
>
> as the commit message is not even remotely acceptable, to pick one
> random example from the series (7/8).
>
> But that doesn't matter much, since I think the more fundamental
> problems are much worse:
>
>  - the whole "ON_BEHALF_OF_KERNEL" and "ON_BEHALF_OF_USERSPACE" is
> just complete noise and totally illogical. The whole concept needs to
> be redone.
>
> Example of complete nonsense (again, picking 7/8, but that's again
> just a random example):
>
> > @@ -3017,8 +3022,8 @@ SYSCALL_DEFINE5(remap_file_pages,
> >                 flags |= MAP_LOCKED;
> >
> >         file = get_file(vma->vm_file);
> > -       ret = do_mmap(vma->vm_file, start, size,
> > -                       prot, flags, pgoff, &populate, NULL);
> > +       ret = do_mmap(vma->vm_file, start, size, prot, flags, pgoff,
> > +                       &populate, NULL, ON_BEHALF_OF_KERNEL);
> >         fput(file);
> >  out:
> >         mmap_write_unlock(mm);
>
> Christ. That's *literally* the remap_file_pages() system call
> definition. No way in hell does "ON_BEHALF_OF_KERNEL" make any sense
> in this context.
>
> It's not the only situation. "mremap() as the same thing. vm_munmap()
> has the same thing. vm_brk_flags() has the same thing. None of them
> make any sense.
>
> But even if those obvious kinds of complete mis-designs were to be
> individually fixed, the whole naming and concept is bogus. The
> "ON_BEHALF_OF_KERNEL" thing seems to actually just mean "don't check
> sealing". Naming should describe what the thing *means*, not some
> random policy thing that may or may not be at all relevant.
>

I apologize that I didn't think of a better name for ON_BEHALF_OF_XX
and I should have written a more clear commit message.

I prepared a V2 patchset with a more detailed commit message, hopefully
that will help to explain the design.

Indeed, the ON_BEHALF_OF_XX is confusing, especially with
remap_file_pathes(). remap_file_pathes(2) is not supported
in this patch set, this covers mprotect(2), mmap(2), munmap(2),
mremap(2) as the first feature set. I could extend the sealing
to more syscalls, if it is determined necessary from the outcome of
this discussion. The initial set of 4 syscalls was chosen based
on Chrome's initial wish list.

Regarding the ON_BEHALF_OF flag, my intention is to have a flag,
set at syscall entry points of mprotect(2), munmap(2), mremap(2),
mmap(2),  pass the flag along the call stack, till reaching
can_modify_mm(), can_modify_mm() does the actual check for
the sealing type.

It is probably worth noting that I choose to check one and only
one sealing type per syscall. i.e. munmap(2) checks
MM_SEAL_MUNMAP only. With this approach, sealing can be
implemented incrementally. For example, When implementing
sealing for munmap(2), I don't need to care that mremap(2)
can also call internal functions to unmap the VMAs.
The mremap(2) will be sealed by MM_SEAL_MREMAP(), and
be dealt with separately.

This approach also allows dev to expand the sealing to madvice(),
mlock(), or whatever syscalls or cases that modify VMA's meta data.
As Yann points out, There is a list of cases that we might care about.
Having all of those implemented will take time. Using bitmasks will
help to add those incrementally. An application will  be backward
compatible when a new sealing type is added, i.e. It has to set the
new sealing type explicitly.

>  - the whole MM_SEAL_xx vs VM_SEAL_xx artificial distinction needs to go away.
>
> Not only is it unacceptable to pointlessly create two different name
> spaces for no obvious reason, code like this (from 1/8) should not
> exist:
>
> > +       if (types & MM_SEAL_MSEAL)
> > +               newtypes |= VM_SEAL_MSEAL;
> > +
> > +       if (types & MM_SEAL_MPROTECT)
> > +               newtypes |= VM_SEAL_MPROTECT;
> > +
> > +       if (types & MM_SEAL_MUNMAP)
> > +               newtypes |= VM_SEAL_MUNMAP;
> > +
> > +       if (types & MM_SEAL_MMAP)
> > +               newtypes |= VM_SEAL_MMAP;
> > +
> > +       if (types & MM_SEAL_MREMAP)
> > +               newtypes |= VM_SEAL_MREMAP;
>
> Sure, we have that in some cases when there was a *reason* for
> namespacing imposed on us from an API standpoint (ie the "open()"
> flags that get turned into FMODE_xyz flags, or the MS_xyz mount flags
> being turned into MNT_xyz flags), but those tend to be signs of
> problems in the system call API where some mixup made it impossible to
> use the flags directly (most commonly because there is some extended
> internal representation of said things).
>
> For example, the MS_xyz namespace is a combination of "these are flags
> for the new mount" (like MS_RDONLY) and "this is how you should mount
> it" (like MS_REMOUNT), and so the user interface has that odd mixing
> of different things, and the MNT_xyz namespace is a distillation of
> the former.
>
> But we certainly should not strive to introduce *new* interfaces that
> start out with this kind of mixup and pointless "translate from one
> bit to another" code.
>
The two namespaces can go away, that means the bitmap will be stored
as is by vm_seals in VMA struct. (1/8) Copied below.

+++ b/include/linux/mm_types.h
@@ -660,6 +660,13 @@ struct vm_area_struct {
+       unsigned long vm_seals;         /* seal flags, see mm.h. */

My original considerations are:
1. vm_seals is a new field, and mseal() currently uses 5 bits.
vm_seals can be repurposed to store other VMA flags in future,
having two namespaces (API and internal one) can be useful.
2. vm_flags is full and it seems to me there is pending work on expanding
vm_flags. [1]  if that happens, we will need translation logic.
[1] https://lore.kernel.org/linux-mm/4F6CA298.4000301@jp.fujitsu.com/

I might have over-engineered this, so I removed the VM_SEAL_XX in V2.

>  - And finally (for now), I hate that MM_ACTION_xyz thing too!
>
> Why do we have MM_ACTION_MREMAP, and pointless like this (from 3/8):
>
> > +       switch (action) {
> > +       case MM_ACTION_MPROTECT:
> > +               if (vma->vm_seals & VM_SEAL_MPROTECT)
> > +                       return false;
> > +               break;
>
> when the sane thing to do is to use the *same* MM_SEAL_xyz bitmask and
> sealing bitmask and just say
>
>         if (vma->vm_seal & vm_action)
>                 return -EPERM;
>

Make sense. My original thought is that can_modify_vma() will check one
and only one seal type, and having an enum type will enforce that.  This
restriction feels unnecessary. I removed the action type in V2.

> IOW, you have pointlessly introduced not *two* different namespaces,
> but *three*. All doing the exact same thing, and all just causing
> pointless and ugly code to "translate" the actions of one into the
> model of another.
>
> So get rid of the "MM_ACTION_xyz" thing. Get rid of ther "VM_SEAL_xyz"
> thing. Use *one* single "these are the sealing bits".
>
> And get rid of "enum caller_origin" entirely. I don't know what the
> right model for that thing is, but that isn't it.
>
> *Maybe* the right model is some MM_SEAL_OVERRIDE bit that user space
> cannot set, but that the kernel can use internally - and if that is
> the right model, then dammit, the *uses* should be very very obvious
> why the override is fine, because that remap_file_pages() use sure as
> hell was *not* obvious.
>
> In fact, it's not at all obvious why anything should override the
> sealing bits - EVER. So I'm not convinced that the right model is
> "replace ON_BEHALF_OF_KERNEL with MM_SEAL_OVERRIDE". Why would we
> *ever* want to override sealing? It sounds like complete garbage. Most
> of the users seem to be things like "execve()", which is nonsensical,
> since the VM wouldn't have been sealed at that point _anyway_, so
> having a "don't bother checking sealing bits" flag seems entirely
> useless.
>
Would the new commit message and comments in V2 help to
explain the design better ? (will send shortly)

Another code change I can make to help the readability (not in v2),
is to set and pass checkSeals flag from syscall entry point, all the
way to can_modify_vma(). Currently, I didn't do that if I checked the
internal function is only used by syscal entry point, e.g. in
do_mprotect_pkey(), mremap_to(), ksys_mmap_pgoff() cases.
Doing that does increase the size of the patch set though.

Thanks.
-Jeff


-Jeff

> Anyway, this is all a resounding NAK on this series in this form. My
> complaints are not some kind of small "fix this up". These are
> fundamental issues.
>
>             Linus

Powered by blists - more mailing lists