| Message ID | 20230119160344.54358-2-joey.gouly@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: In-kernel support for memory-deny-write-execute (MDWE) | expand |
On 19.01.23 17:03, Joey Gouly wrote: > The aim of such policy is to prevent a user task from creating an > executable mapping that is also writeable. > > An example of mmap() returning -EACCESS if the policy is enabled: > > mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC, flags, 0, 0); > > Similarly, mprotect() would return -EACCESS below: > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > mprotect(addr, size, PROT_READ | PROT_WRITE | PROT_EXEC); > > The BPF filter that systemd MDWE uses is stateless, and disallows > mprotect() with PROT_EXEC completely. This new prctl allows PROT_EXEC to > be enabled if it was already PROT_EXEC, which allows the following case: > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); > > where PROT_BTI enables branch tracking identification on arm64. > > Signed-off-by: Joey Gouly <joey.gouly@arm.com> > Co-developed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > --- > include/linux/mman.h | 34 ++++++++++++++++++++++++++++++++++ > include/linux/sched/coredump.h | 6 +++++- > include/uapi/linux/prctl.h | 6 ++++++ > kernel/sys.c | 33 +++++++++++++++++++++++++++++++++ > mm/mmap.c | 10 ++++++++++ > mm/mprotect.c | 5 +++++ > 6 files changed, 93 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 58b3abd457a3..cee1e4b566d8 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) > } > > unsigned long vm_commit_limit(void); > + > +/* > + * Denies creating a writable executable mapping or gaining executable permissions. > + * > + * This denies the following: > + * > + * a) mmap(PROT_WRITE | PROT_EXEC) > + * > + * b) mmap(PROT_WRITE) > + * mprotect(PROT_EXEC) > + * > + * c) mmap(PROT_WRITE) > + * mprotect(PROT_READ) > + * mprotect(PROT_EXEC) > + * > + * But allows the following: > + * > + * d) mmap(PROT_READ | PROT_EXEC) > + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) > + */ Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC anymore? In an ideal world, there would be no further mprotect changes required.
On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote: > On 19.01.23 17:03, Joey Gouly wrote: > > The aim of such policy is to prevent a user task from creating an > > executable mapping that is also writeable. > > > > An example of mmap() returning -EACCESS if the policy is enabled: > > > > mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC, flags, 0, 0); > > > > Similarly, mprotect() would return -EACCESS below: > > > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > > mprotect(addr, size, PROT_READ | PROT_WRITE | PROT_EXEC); > > > > The BPF filter that systemd MDWE uses is stateless, and disallows > > mprotect() with PROT_EXEC completely. This new prctl allows PROT_EXEC to > > be enabled if it was already PROT_EXEC, which allows the following case: > > > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > > mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); > > > > where PROT_BTI enables branch tracking identification on arm64. > > > > Signed-off-by: Joey Gouly <joey.gouly@arm.com> > > Co-developed-by: Catalin Marinas <catalin.marinas@arm.com> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > --- > > include/linux/mman.h | 34 ++++++++++++++++++++++++++++++++++ > > include/linux/sched/coredump.h | 6 +++++- > > include/uapi/linux/prctl.h | 6 ++++++ > > kernel/sys.c | 33 +++++++++++++++++++++++++++++++++ > > mm/mmap.c | 10 ++++++++++ > > mm/mprotect.c | 5 +++++ > > 6 files changed, 93 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/mman.h b/include/linux/mman.h > > index 58b3abd457a3..cee1e4b566d8 100644 > > --- a/include/linux/mman.h > > +++ b/include/linux/mman.h > > @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) > > } > > unsigned long vm_commit_limit(void); > > + > > +/* > > + * Denies creating a writable executable mapping or gaining executable permissions. > > + * > > + * This denies the following: > > + * > > + * a) mmap(PROT_WRITE | PROT_EXEC) > > + * > > + * b) mmap(PROT_WRITE) > > + * mprotect(PROT_EXEC) > > + * > > + * c) mmap(PROT_WRITE) > > + * mprotect(PROT_READ) > > + * mprotect(PROT_EXEC) > > + * > > + * But allows the following: > > + * > > + * d) mmap(PROT_READ | PROT_EXEC) > > + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) > > + */ > > Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC > anymore? In an ideal world, there would be no further mprotect changes > required. I don't think it works for this scenario. We don't want to disable PROT_EXEC entirely, only disallow it if the mapping is not already executable. The below should be allowed: addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap() time. We could clear VM_MAYEXEC if the mapping was made VM_WRITE (either by mmap() or mprotect()) but IIRC we concluded that this should be an additional prctl() flag. This series aims to be pretty much a drop-in replacement for the systemd's MDWE SECCOMP feature (but allowing PROT_BTI).
On 23.01.23 13:19, Catalin Marinas wrote: > On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote: >> On 19.01.23 17:03, Joey Gouly wrote: >>> The aim of such policy is to prevent a user task from creating an >>> executable mapping that is also writeable. >>> >>> An example of mmap() returning -EACCESS if the policy is enabled: >>> >>> mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC, flags, 0, 0); >>> >>> Similarly, mprotect() would return -EACCESS below: >>> >>> addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); >>> mprotect(addr, size, PROT_READ | PROT_WRITE | PROT_EXEC); >>> >>> The BPF filter that systemd MDWE uses is stateless, and disallows >>> mprotect() with PROT_EXEC completely. This new prctl allows PROT_EXEC to >>> be enabled if it was already PROT_EXEC, which allows the following case: >>> >>> addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); >>> mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); >>> >>> where PROT_BTI enables branch tracking identification on arm64. >>> >>> Signed-off-by: Joey Gouly <joey.gouly@arm.com> >>> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com> >>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Andrew Morton <akpm@linux-foundation.org> >>> --- >>> include/linux/mman.h | 34 ++++++++++++++++++++++++++++++++++ >>> include/linux/sched/coredump.h | 6 +++++- >>> include/uapi/linux/prctl.h | 6 ++++++ >>> kernel/sys.c | 33 +++++++++++++++++++++++++++++++++ >>> mm/mmap.c | 10 ++++++++++ >>> mm/mprotect.c | 5 +++++ >>> 6 files changed, 93 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/linux/mman.h b/include/linux/mman.h >>> index 58b3abd457a3..cee1e4b566d8 100644 >>> --- a/include/linux/mman.h >>> +++ b/include/linux/mman.h >>> @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) >>> } >>> unsigned long vm_commit_limit(void); >>> + >>> +/* >>> + * Denies creating a writable executable mapping or gaining executable permissions. >>> + * >>> + * This denies the following: >>> + * >>> + * a) mmap(PROT_WRITE | PROT_EXEC) >>> + * >>> + * b) mmap(PROT_WRITE) >>> + * mprotect(PROT_EXEC) >>> + * >>> + * c) mmap(PROT_WRITE) >>> + * mprotect(PROT_READ) >>> + * mprotect(PROT_EXEC) >>> + * >>> + * But allows the following: >>> + * >>> + * d) mmap(PROT_READ | PROT_EXEC) >>> + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) >>> + */ >> >> Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC >> anymore? In an ideal world, there would be no further mprotect changes >> required. > > I don't think it works for this scenario. We don't want to disable > PROT_EXEC entirely, only disallow it if the mapping is not already > executable. The below should be allowed: > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); > > but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap() > time. Yeah, if you allow write access at mmap time, clear VM_MAYEXEC (and disallow VM_EXEC of course). But I guess we'd have to go one step further: if we allow exec access at mmap time, clear VM_MAYWRITE (and disallow VM_WRITE of course). That at least would be then similar to how we handle mmaped files: if the file is not executable, we clear VM_MAYEXEC. If the file is not writable, we clear VM_MAYWRITE. Clearing VM_MAYWRITE would imply that also writes via /proc/self/mem to such memory would be forbidden, which might also be what we are trying to achieve, or is that expected to still work? But clearing VM_MAYWRITE would mean that is_cow_mapping() would no longer fire for some VMAs, and we'd have to check if that's fine in all cases. Having that said, this patch handles the case when the prctl is applied to a process after already having created some writable or executable mappings, to at least forbid if afterwards on these mappings. What is expected to happen if the process already has writable mappings that are executable at the time we enable the prctl? Clarifying what the expected semantics with /proc/self/mem are would be nice. > > We could clear VM_MAYEXEC if the mapping was made VM_WRITE (either by > mmap() or mprotect()) but IIRC we concluded that this should be an > additional prctl() flag. Yes, I agree with enabling this restriction on a per-process level. My remarks only apply to processes with this toggle enabled.
On Mon, Jan 23, 2023 at 01:53:46PM +0100, David Hildenbrand wrote: > On 23.01.23 13:19, Catalin Marinas wrote: > > On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote: > > > On 19.01.23 17:03, Joey Gouly wrote: > > > > diff --git a/include/linux/mman.h b/include/linux/mman.h > > > > index 58b3abd457a3..cee1e4b566d8 100644 > > > > --- a/include/linux/mman.h > > > > +++ b/include/linux/mman.h > > > > @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) > > > > } > > > > unsigned long vm_commit_limit(void); > > > > + > > > > +/* > > > > + * Denies creating a writable executable mapping or gaining executable permissions. > > > > + * > > > > + * This denies the following: > > > > + * > > > > + * a) mmap(PROT_WRITE | PROT_EXEC) > > > > + * > > > > + * b) mmap(PROT_WRITE) > > > > + * mprotect(PROT_EXEC) > > > > + * > > > > + * c) mmap(PROT_WRITE) > > > > + * mprotect(PROT_READ) > > > > + * mprotect(PROT_EXEC) > > > > + * > > > > + * But allows the following: > > > > + * > > > > + * d) mmap(PROT_READ | PROT_EXEC) > > > > + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) > > > > + */ > > > > > > Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC > > > anymore? In an ideal world, there would be no further mprotect changes > > > required. > > > > I don't think it works for this scenario. We don't want to disable > > PROT_EXEC entirely, only disallow it if the mapping is not already > > executable. The below should be allowed: > > > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > > mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); > > > > but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap() > > time. > > Yeah, if you allow write access at mmap time, clear VM_MAYEXEC (and disallow > VM_EXEC of course). This should work but it doesn't fully mimic systemd's MDWE behaviour (e.g. disallow mprotect(PROT_EXEC) even if the mmap was PROT_READ only). Topi wanted to stay close to that at least in the first incarnation of this control (can be extended later). > But I guess we'd have to go one step further: if we allow exec access > at mmap time, clear VM_MAYWRITE (and disallow VM_WRITE of course). Yes, both this and the VM_MAYEXEC clearing if VM_WRITE would be useful but as additional controls a process can enable. > That at least would be then similar to how we handle mmaped files: if the > file is not executable, we clear VM_MAYEXEC. If the file is not writable, we > clear VM_MAYWRITE. We still allow VM_MAYWRITE for private mappings, though we do clear VM_MAYEXEC if not executable. It would be nice to use VM_MAY* flags for this logic but we can only emulate MDWE if we change the semantics of 'MAY': only check the 'MAY' flags for permissions being changed (e.g. allow PROT_EXEC if the vma is already VM_EXEC even if !VM_MAYEXEC). Another issue is that we end up with some weird combinations like having VM_EXEC without VM_MAYEXEC (maybe that's fine). > Clearing VM_MAYWRITE would imply that also writes via /proc/self/mem to such > memory would be forbidden, which might also be what we are trying to > achieve, or is that expected to still work? I think currently with systemd's MDWE it still works (I haven't tried though), unless there's something else forcing that file read-only. > But clearing VM_MAYWRITE would mean that is_cow_mapping() would no > longer fire for some VMAs, and we'd have to check if that's fine in > all cases. This will break __access_remote_vm() AFAICT since it can't do a CoW on read-only private mapping. > Having that said, this patch handles the case when the prctl is applied to a > process after already having created some writable or executable mappings, > to at least forbid if afterwards on these mappings. What is expected to > happen if the process already has writable mappings that are executable at > the time we enable the prctl? They are expected to continue to work. The prctl() is meant to be invoked by something like systemd so that any subsequent exec() will inherit the property. > Clarifying what the expected semantics with /proc/self/mem are would be > nice. Yeah, this series doesn't handle this. Topi, do you know if systemd does anything about /proc/self/mem? To me this option is more about catching inadvertent write|exec mappings rather than blocking programs that insist on doing this (they can always map a memfd file twice with separate write and exec attributes for example).
On 23.01.23 17:04, Catalin Marinas wrote: > On Mon, Jan 23, 2023 at 01:53:46PM +0100, David Hildenbrand wrote: >> On 23.01.23 13:19, Catalin Marinas wrote: >>> On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote: >>>> On 19.01.23 17:03, Joey Gouly wrote: >>>>> diff --git a/include/linux/mman.h b/include/linux/mman.h >>>>> index 58b3abd457a3..cee1e4b566d8 100644 >>>>> --- a/include/linux/mman.h >>>>> +++ b/include/linux/mman.h >>>>> @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) >>>>> } >>>>> unsigned long vm_commit_limit(void); >>>>> + >>>>> +/* >>>>> + * Denies creating a writable executable mapping or gaining executable permissions. >>>>> + * >>>>> + * This denies the following: >>>>> + * >>>>> + * a) mmap(PROT_WRITE | PROT_EXEC) >>>>> + * >>>>> + * b) mmap(PROT_WRITE) >>>>> + * mprotect(PROT_EXEC) >>>>> + * >>>>> + * c) mmap(PROT_WRITE) >>>>> + * mprotect(PROT_READ) >>>>> + * mprotect(PROT_EXEC) >>>>> + * >>>>> + * But allows the following: >>>>> + * >>>>> + * d) mmap(PROT_READ | PROT_EXEC) >>>>> + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) >>>>> + */ >>>> >>>> Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC >>>> anymore? In an ideal world, there would be no further mprotect changes >>>> required. >>> >>> I don't think it works for this scenario. We don't want to disable >>> PROT_EXEC entirely, only disallow it if the mapping is not already >>> executable. The below should be allowed: >>> >>> addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); >>> mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); >>> >>> but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap() >>> time. >> >> Yeah, if you allow write access at mmap time, clear VM_MAYEXEC (and disallow >> VM_EXEC of course). > > This should work but it doesn't fully mimic systemd's MDWE behaviour > (e.g. disallow mprotect(PROT_EXEC) even if the mmap was PROT_READ only). Interesting. > Topi wanted to stay close to that at least in the first incarnation of > this control (can be extended later). > >> But I guess we'd have to go one step further: if we allow exec access >> at mmap time, clear VM_MAYWRITE (and disallow VM_WRITE of course). > > Yes, both this and the VM_MAYEXEC clearing if VM_WRITE would be useful > but as additional controls a process can enable. > >> That at least would be then similar to how we handle mmaped files: if the >> file is not executable, we clear VM_MAYEXEC. If the file is not writable, we >> clear VM_MAYWRITE. > > We still allow VM_MAYWRITE for private mappings, though we do clear > VM_MAYEXEC if not executable. > > It would be nice to use VM_MAY* flags for this logic but we can only > emulate MDWE if we change the semantics of 'MAY': only check the 'MAY' > flags for permissions being changed (e.g. allow PROT_EXEC if the vma is > already VM_EXEC even if !VM_MAYEXEC). Another issue is that we end up > with some weird combinations like having VM_EXEC without VM_MAYEXEC > (maybe that's fine). No, we wouldn't want VM_EXEC if VM_MAYEXEC is not set. I don't immediately see how that would happen. > >> Clearing VM_MAYWRITE would imply that also writes via /proc/self/mem to such >> memory would be forbidden, which might also be what we are trying to >> achieve, or is that expected to still work? > > I think currently with systemd's MDWE it still works (I haven't tried > though), unless there's something else forcing that file read-only. Okay, just curious if this is an easy way to bypass the MDWE restriction. > >> But clearing VM_MAYWRITE would mean that is_cow_mapping() would no >> longer fire for some VMAs, and we'd have to check if that's fine in >> all cases. > > This will break __access_remote_vm() AFAICT since it can't do a CoW on > read-only private mapping. Yeah, might require some thought. > >> Having that said, this patch handles the case when the prctl is applied to a >> process after already having created some writable or executable mappings, >> to at least forbid if afterwards on these mappings. What is expected to >> happen if the process already has writable mappings that are executable at >> the time we enable the prctl? > > They are expected to continue to work. The prctl() is meant to be > invoked by something like systemd so that any subsequent exec() will > inherit the property. Okay, thanks. So it's mostly about new processes inheriting that restriction. > >> Clarifying what the expected semantics with /proc/self/mem are would be >> nice. > > Yeah, this series doesn't handle this. Topi, do you know if systemd does > anything about /proc/self/mem? To me this option is more about catching > inadvertent write|exec mappings rather than blocking programs that > insist on doing this (they can always map a memfd file twice with > separate write and exec attributes for example). I remember some work regarding forbidding ececutable memfds.
On Mon, Jan 23, 2023 at 05:10:08PM +0100, David Hildenbrand wrote: > On 23.01.23 17:04, Catalin Marinas wrote: > > On Mon, Jan 23, 2023 at 01:53:46PM +0100, David Hildenbrand wrote: > > > That at least would be then similar to how we handle mmaped files: if the > > > file is not executable, we clear VM_MAYEXEC. If the file is not writable, we > > > clear VM_MAYWRITE. > > > > We still allow VM_MAYWRITE for private mappings, though we do clear > > VM_MAYEXEC if not executable. > > > > It would be nice to use VM_MAY* flags for this logic but we can only > > emulate MDWE if we change the semantics of 'MAY': only check the 'MAY' > > flags for permissions being changed (e.g. allow PROT_EXEC if the vma is > > already VM_EXEC even if !VM_MAYEXEC). Another issue is that we end up > > with some weird combinations like having VM_EXEC without VM_MAYEXEC > > (maybe that's fine). > > No, we wouldn't want VM_EXEC if VM_MAYEXEC is not set. I don't immediately > see how that would happen. You are right, this shouldn't happen. What I had in mind was the current MDWE model where after an mmap(PROT_EXEC), any mprotect(PROT_EXEC) is denied. But this series departs slightly from this since we want to allow PROT_EXEC if already executable.
On 23.1.2023 18.04, Catalin Marinas wrote: > On Mon, Jan 23, 2023 at 01:53:46PM +0100, David Hildenbrand wrote: >> On 23.01.23 13:19, Catalin Marinas wrote: >>> On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote: >>>> On 19.01.23 17:03, Joey Gouly wrote: >>>>> diff --git a/include/linux/mman.h b/include/linux/mman.h >>>>> index 58b3abd457a3..cee1e4b566d8 100644 >>>>> --- a/include/linux/mman.h >>>>> +++ b/include/linux/mman.h >>>>> @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) >>>>> } >>>>> unsigned long vm_commit_limit(void); >>>>> + >>>>> +/* >>>>> + * Denies creating a writable executable mapping or gaining executable permissions. >>>>> + * >>>>> + * This denies the following: >>>>> + * >>>>> + * a) mmap(PROT_WRITE | PROT_EXEC) >>>>> + * >>>>> + * b) mmap(PROT_WRITE) >>>>> + * mprotect(PROT_EXEC) >>>>> + * >>>>> + * c) mmap(PROT_WRITE) >>>>> + * mprotect(PROT_READ) >>>>> + * mprotect(PROT_EXEC) >>>>> + * >>>>> + * But allows the following: >>>>> + * >>>>> + * d) mmap(PROT_READ | PROT_EXEC) >>>>> + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) >>>>> + */ >>>> >>>> Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC >>>> anymore? In an ideal world, there would be no further mprotect changes >>>> required. >>> >>> I don't think it works for this scenario. We don't want to disable >>> PROT_EXEC entirely, only disallow it if the mapping is not already >>> executable. The below should be allowed: >>> >>> addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); >>> mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); >>> >>> but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap() >>> time. >> >> Yeah, if you allow write access at mmap time, clear VM_MAYEXEC (and disallow >> VM_EXEC of course). > > This should work but it doesn't fully mimic systemd's MDWE behaviour > (e.g. disallow mprotect(PROT_EXEC) even if the mmap was PROT_READ only). > Topi wanted to stay close to that at least in the first incarnation of > this control (can be extended later). > >> But I guess we'd have to go one step further: if we allow exec access >> at mmap time, clear VM_MAYWRITE (and disallow VM_WRITE of course). > > Yes, both this and the VM_MAYEXEC clearing if VM_WRITE would be useful > but as additional controls a process can enable. > >> That at least would be then similar to how we handle mmaped files: if the >> file is not executable, we clear VM_MAYEXEC. If the file is not writable, we >> clear VM_MAYWRITE. > > We still allow VM_MAYWRITE for private mappings, though we do clear > VM_MAYEXEC if not executable. > > It would be nice to use VM_MAY* flags for this logic but we can only > emulate MDWE if we change the semantics of 'MAY': only check the 'MAY' > flags for permissions being changed (e.g. allow PROT_EXEC if the vma is > already VM_EXEC even if !VM_MAYEXEC). Another issue is that we end up > with some weird combinations like having VM_EXEC without VM_MAYEXEC > (maybe that's fine). > >> Clearing VM_MAYWRITE would imply that also writes via /proc/self/mem to such >> memory would be forbidden, which might also be what we are trying to >> achieve, or is that expected to still work? > > I think currently with systemd's MDWE it still works (I haven't tried > though), unless there's something else forcing that file read-only. > >> But clearing VM_MAYWRITE would mean that is_cow_mapping() would no >> longer fire for some VMAs, and we'd have to check if that's fine in >> all cases. > > This will break __access_remote_vm() AFAICT since it can't do a CoW on > read-only private mapping. > >> Having that said, this patch handles the case when the prctl is applied to a >> process after already having created some writable or executable mappings, >> to at least forbid if afterwards on these mappings. What is expected to >> happen if the process already has writable mappings that are executable at >> the time we enable the prctl? > > They are expected to continue to work. The prctl() is meant to be > invoked by something like systemd so that any subsequent exec() will > inherit the property. > >> Clarifying what the expected semantics with /proc/self/mem are would be >> nice. > > Yeah, this series doesn't handle this. Topi, do you know if systemd does > anything about /proc/self/mem? To me this option is more about catching > inadvertent write|exec mappings rather than blocking programs that > insist on doing this (they can always map a memfd file twice with > separate write and exec attributes for example). > I don't think so. For 100% compatibility with seccomp, the same cases of mprotect() use should be blocked regardless of the file descriptor used. There could be more relaxed PR_MDWE_* controls in the future if needed. Updated systemd PR: https://github.com/systemd/systemd/pull/25276 I wish there were highly granular access controls for /proc, including /proc/self and /proc/sys/*. Now the best options are to use mount namespaces and/or SELinux, but they aren't too good for that. -Topi
On 2023-01-19 19:03, Joey Gouly wrote: > The aim of such policy is to prevent a user task from creating an > executable mapping that is also writeable. > > An example of mmap() returning -EACCESS if the policy is enabled: > > mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC, flags, 0, 0); > > Similarly, mprotect() would return -EACCESS below: > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > mprotect(addr, size, PROT_READ | PROT_WRITE | PROT_EXEC); > > The BPF filter that systemd MDWE uses is stateless, and disallows > mprotect() with PROT_EXEC completely. This new prctl allows PROT_EXEC > to > be enabled if it was already PROT_EXEC, which allows the following > case: > > addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0); > mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI); > > where PROT_BTI enables branch tracking identification on arm64. > > Signed-off-by: Joey Gouly <joey.gouly@arm.com> > Co-developed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > --- > include/linux/mman.h | 34 ++++++++++++++++++++++++++++++++++ > include/linux/sched/coredump.h | 6 +++++- > include/uapi/linux/prctl.h | 6 ++++++ > kernel/sys.c | 33 +++++++++++++++++++++++++++++++++ > mm/mmap.c | 10 ++++++++++ > mm/mprotect.c | 5 +++++ > 6 files changed, 93 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 58b3abd457a3..cee1e4b566d8 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) > } > > unsigned long vm_commit_limit(void); > + > +/* > + * Denies creating a writable executable mapping or gaining > executable permissions. > + * > + * This denies the following: > + * > + * a) mmap(PROT_WRITE | PROT_EXEC) > + * > + * b) mmap(PROT_WRITE) > + * mprotect(PROT_EXEC) > + * > + * c) mmap(PROT_WRITE) > + * mprotect(PROT_READ) > + * mprotect(PROT_EXEC) > + * > + * But allows the following: > + * > + * d) mmap(PROT_READ | PROT_EXEC) > + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) > + */ > +static inline bool map_deny_write_exec(struct vm_area_struct *vma, > unsigned long vm_flags) > +{ > + if (!test_bit(MMF_HAS_MDWE, ¤t->mm->flags)) > + return false; > + > + if ((vm_flags & VM_EXEC) && (vm_flags & VM_WRITE)) > + return true; > + > + if (!(vma->vm_flags & VM_EXEC) && (vm_flags & VM_EXEC)) > + return true; > + > + return false; > +} > + > #endif /* _LINUX_MMAN_H */ > diff --git a/include/linux/sched/coredump.h > b/include/linux/sched/coredump.h > index 8270ad7ae14c..0e17ae7fbfd3 100644 > --- a/include/linux/sched/coredump.h > +++ b/include/linux/sched/coredump.h > @@ -81,9 +81,13 @@ static inline int get_dumpable(struct mm_struct *mm) > * lifecycle of this mm, just for simplicity. > */ > #define MMF_HAS_PINNED 27 /* FOLL_PIN has run, never cleared */ > + > +#define MMF_HAS_MDWE 28 > +#define MMF_HAS_MDWE_MASK (1 << MMF_HAS_MDWE) > + > #define MMF_DISABLE_THP_MASK (1 << MMF_DISABLE_THP) > > #define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK |\ > - MMF_DISABLE_THP_MASK) > + MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK) > > #endif /* _LINUX_SCHED_COREDUMP_H */ > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h > index a5e06dcbba13..1312a137f7fb 100644 > --- a/include/uapi/linux/prctl.h > +++ b/include/uapi/linux/prctl.h > @@ -281,6 +281,12 @@ struct prctl_mm_map { > # define PR_SME_VL_LEN_MASK 0xffff > # define PR_SME_VL_INHERIT (1 << 17) /* inherit across exec */ > > +/* Memory deny write / execute */ > +#define PR_SET_MDWE 65 > +# define PR_MDWE_REFUSE_EXEC_GAIN 1 > + > +#define PR_GET_MDWE 66 > + > #define PR_SET_VMA 0x53564d41 > # define PR_SET_VMA_ANON_NAME 0 > > diff --git a/kernel/sys.c b/kernel/sys.c > index 5fd54bf0e886..b3cab94545ed 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2348,6 +2348,33 @@ static int prctl_set_vma(unsigned long opt, > unsigned long start, > } > #endif /* CONFIG_ANON_VMA_NAME */ > > +static inline int prctl_set_mdwe(unsigned long bits, unsigned long > arg3, > + unsigned long arg4, unsigned long arg5) > +{ > + if (arg3 || arg4 || arg5) > + return -EINVAL; > + > + if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN)) > + return -EINVAL; > + > + if (bits & PR_MDWE_REFUSE_EXEC_GAIN) > + set_bit(MMF_HAS_MDWE, ¤t->mm->flags); > + else if (test_bit(MMF_HAS_MDWE, ¤t->mm->flags)) > + return -EPERM; /* Cannot unset the flag */ > + > + return 0; > +} > + > +static inline int prctl_get_mdwe(unsigned long arg2, unsigned long > arg3, > + unsigned long arg4, unsigned long arg5) > +{ > + if (arg2 || arg3 || arg4 || arg5) > + return -EINVAL; > + > + return test_bit(MMF_HAS_MDWE, ¤t->mm->flags) ? > + PR_MDWE_REFUSE_EXEC_GAIN : 0; > +} > + > SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned > long, arg3, > unsigned long, arg4, unsigned long, arg5) > { > @@ -2623,6 +2650,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned > long, arg2, unsigned long, arg3, > error = sched_core_share_pid(arg2, arg3, arg4, arg5); > break; > #endif > + case PR_SET_MDWE: > + error = prctl_set_mdwe(arg2, arg3, arg4, arg5); > + break; > + case PR_GET_MDWE: > + error = prctl_get_mdwe(arg2, arg3, arg4, arg5); > + break; > case PR_SET_VMA: > error = prctl_set_vma(arg2, arg3, arg4, arg5); > break; > diff --git a/mm/mmap.c b/mm/mmap.c > index 87d929316d57..99a4d9e2b0d8 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2665,6 +2665,16 @@ unsigned long mmap_region(struct file *file, > unsigned long addr, > vma_set_anonymous(vma); > } > > + if (map_deny_write_exec(vma, vma->vm_flags)) { > + error = -EACCES; > + if (file) > + goto close_and_free_vma; > + else if (vma->vm_file) > + goto unmap_and_free_vma; > + else > + goto free_vma; > + } > + Why is the cleanup dispatch logic duplicated here, instead of simply doing "goto close_and_free_vma" (where basically the same dispatch is done)? > /* Allow architectures to sanity-check the vm_flags */ > if (!arch_validate_flags(vma->vm_flags)) { > error = -EINVAL; > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 908df12caa26..bc0587df042f 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -762,6 +762,11 @@ static int do_mprotect_pkey(unsigned long start, > size_t len, > break; > } > > + if (map_deny_write_exec(vma, newflags)) { > + error = -EACCES; > + goto out; > + } > + Why does this check use "goto out", thereby skipping post-loop cleanup, instead of "break" like all other checks? This looks like a bug to me. Alexey > /* Allow architectures to sanity-check the new flags */ > if (!arch_validate_flags(newflags)) { > error = -EINVAL;
On Tue, Mar 07, 2023 at 04:01:56PM +0300, Alexey Izbyshev wrote: > On 2023-01-19 19:03, Joey Gouly wrote: > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 87d929316d57..99a4d9e2b0d8 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -2665,6 +2665,16 @@ unsigned long mmap_region(struct file *file, > > unsigned long addr, > > vma_set_anonymous(vma); > > } > > > > + if (map_deny_write_exec(vma, vma->vm_flags)) { > > + error = -EACCES; > > + if (file) > > + goto close_and_free_vma; > > + else if (vma->vm_file) > > + goto unmap_and_free_vma; > > + else > > + goto free_vma; > > + } > > + > > Why is the cleanup dispatch logic duplicated here, instead of simply doing > "goto close_and_free_vma" (where basically the same dispatch is done)? Yes, though that's only possible after commit cc8d1b097de7 ("mmap: clean up mmap_region() unrolling") in 6.3-rc1. It's worth adding a separate patch to simplify this before final 6.3. > > diff --git a/mm/mprotect.c b/mm/mprotect.c > > index 908df12caa26..bc0587df042f 100644 > > --- a/mm/mprotect.c > > +++ b/mm/mprotect.c > > @@ -762,6 +762,11 @@ static int do_mprotect_pkey(unsigned long start, > > size_t len, > > break; > > } > > > > + if (map_deny_write_exec(vma, newflags)) { > > + error = -EACCES; > > + goto out; > > + } > > + > > Why does this check use "goto out", thereby skipping post-loop cleanup, > instead of "break" like all other checks? This looks like a bug to me. Ah, good point, thanks. I think that's a left-over from my early attempt at this series. The loop was changed in 5.19 with commit 4a18419f71cd ("mm/mprotect: use mmu_gather") but the patch not updated. So yeah, it needs fixing. Joey, could you please send fixes for both issues above? Thanks.
diff --git a/include/linux/mman.h b/include/linux/mman.h index 58b3abd457a3..cee1e4b566d8 100644 --- a/include/linux/mman.h +++ b/include/linux/mman.h @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags) } unsigned long vm_commit_limit(void); + +/* + * Denies creating a writable executable mapping or gaining executable permissions. + * + * This denies the following: + * + * a) mmap(PROT_WRITE | PROT_EXEC) + * + * b) mmap(PROT_WRITE) + * mprotect(PROT_EXEC) + * + * c) mmap(PROT_WRITE) + * mprotect(PROT_READ) + * mprotect(PROT_EXEC) + * + * But allows the following: + * + * d) mmap(PROT_READ | PROT_EXEC) + * mmap(PROT_READ | PROT_EXEC | PROT_BTI) + */ +static inline bool map_deny_write_exec(struct vm_area_struct *vma, unsigned long vm_flags) +{ + if (!test_bit(MMF_HAS_MDWE, ¤t->mm->flags)) + return false; + + if ((vm_flags & VM_EXEC) && (vm_flags & VM_WRITE)) + return true; + + if (!(vma->vm_flags & VM_EXEC) && (vm_flags & VM_EXEC)) + return true; + + return false; +} + #endif /* _LINUX_MMAN_H */ diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h index 8270ad7ae14c..0e17ae7fbfd3 100644 --- a/include/linux/sched/coredump.h +++ b/include/linux/sched/coredump.h @@ -81,9 +81,13 @@ static inline int get_dumpable(struct mm_struct *mm) * lifecycle of this mm, just for simplicity. */ #define MMF_HAS_PINNED 27 /* FOLL_PIN has run, never cleared */ + +#define MMF_HAS_MDWE 28 +#define MMF_HAS_MDWE_MASK (1 << MMF_HAS_MDWE) + #define MMF_DISABLE_THP_MASK (1 << MMF_DISABLE_THP) #define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK |\ - MMF_DISABLE_THP_MASK) + MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK) #endif /* _LINUX_SCHED_COREDUMP_H */ diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index a5e06dcbba13..1312a137f7fb 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h @@ -281,6 +281,12 @@ struct prctl_mm_map { # define PR_SME_VL_LEN_MASK 0xffff # define PR_SME_VL_INHERIT (1 << 17) /* inherit across exec */ +/* Memory deny write / execute */ +#define PR_SET_MDWE 65 +# define PR_MDWE_REFUSE_EXEC_GAIN 1 + +#define PR_GET_MDWE 66 + #define PR_SET_VMA 0x53564d41 # define PR_SET_VMA_ANON_NAME 0 diff --git a/kernel/sys.c b/kernel/sys.c index 5fd54bf0e886..b3cab94545ed 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2348,6 +2348,33 @@ static int prctl_set_vma(unsigned long opt, unsigned long start, } #endif /* CONFIG_ANON_VMA_NAME */ +static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3, + unsigned long arg4, unsigned long arg5) +{ + if (arg3 || arg4 || arg5) + return -EINVAL; + + if (bits & ~(PR_MDWE_REFUSE_EXEC_GAIN)) + return -EINVAL; + + if (bits & PR_MDWE_REFUSE_EXEC_GAIN) + set_bit(MMF_HAS_MDWE, ¤t->mm->flags); + else if (test_bit(MMF_HAS_MDWE, ¤t->mm->flags)) + return -EPERM; /* Cannot unset the flag */ + + return 0; +} + +static inline int prctl_get_mdwe(unsigned long arg2, unsigned long arg3, + unsigned long arg4, unsigned long arg5) +{ + if (arg2 || arg3 || arg4 || arg5) + return -EINVAL; + + return test_bit(MMF_HAS_MDWE, ¤t->mm->flags) ? + PR_MDWE_REFUSE_EXEC_GAIN : 0; +} + SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, unsigned long, arg4, unsigned long, arg5) { @@ -2623,6 +2650,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = sched_core_share_pid(arg2, arg3, arg4, arg5); break; #endif + case PR_SET_MDWE: + error = prctl_set_mdwe(arg2, arg3, arg4, arg5); + break; + case PR_GET_MDWE: + error = prctl_get_mdwe(arg2, arg3, arg4, arg5); + break; case PR_SET_VMA: error = prctl_set_vma(arg2, arg3, arg4, arg5); break; diff --git a/mm/mmap.c b/mm/mmap.c index 87d929316d57..99a4d9e2b0d8 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2665,6 +2665,16 @@ unsigned long mmap_region(struct file *file, unsigned long addr, vma_set_anonymous(vma); } + if (map_deny_write_exec(vma, vma->vm_flags)) { + error = -EACCES; + if (file) + goto close_and_free_vma; + else if (vma->vm_file) + goto unmap_and_free_vma; + else + goto free_vma; + } + /* Allow architectures to sanity-check the vm_flags */ if (!arch_validate_flags(vma->vm_flags)) { error = -EINVAL; diff --git a/mm/mprotect.c b/mm/mprotect.c index 908df12caa26..bc0587df042f 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -762,6 +762,11 @@ static int do_mprotect_pkey(unsigned long start, size_t len, break; } + if (map_deny_write_exec(vma, newflags)) { + error = -EACCES; + goto out; + } + /* Allow architectures to sanity-check the new flags */ if (!arch_validate_flags(newflags)) { error = -EINVAL;