On Mon, 16 Oct 2023 at 07:38, <jeffxu@xxxxxxxxxxxx> 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.
- 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.
- 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;
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.
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
