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
| ||
|
Message-ID: <CAHk-=whFZoap+DBTYvJx6ohqPwn11Puzh7q4huFWDX9vBwXHgg@mail.gmail.com> Date: Mon, 16 Oct 2023 10:23:29 -0700 From: Linus Torvalds <torvalds@...ux-foundation.org> To: jeffxu@...omium.org Cc: akpm@...ux-foundation.org, keescook@...omium.org, sroettger@...gle.com, jeffxu@...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 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. - 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
Powered by blists - more mailing lists