linux-kernel - Re: [RFC PATCH v1 2/2] mseal: allow noop mprotect

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKbZUD0M-kfxqD+AxjYK0LjnZhsxyJbGwhL5CGKzfOsFQpnKtg@mail.gmail.com>
Date: Wed, 12 Mar 2025 15:48:31 +0000
From: Pedro Falcato <pedro.falcato@...il.com>
To: Kees Cook <kees@...nel.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, jeffxu@...omium.org, 
	akpm@...ux-foundation.org, vbabka@...e.cz, Liam.Howlett@...cle.com, 
	broonie@...nel.org, skhan@...uxfoundation.org, linux-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-mm@...ck.org, jorgelo@...omium.org, keescook@...omium.org, 
	rdunlap@...radead.org, jannh@...gle.com
Subject: Re: [RFC PATCH v1 2/2] mseal: allow noop mprotect

On Wed, Mar 12, 2025 at 3:28 PM Kees Cook <kees@...nel.org> wrote:
>
>
>
> On March 12, 2025 6:49:39 AM PDT, Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
> >On Wed, Mar 12, 2025 at 12:21:17AM +0000, jeffxu@...omium.org wrote:
> >> From: Jeff Xu <jeffxu@...omium.org>
> >>
> >> Initially, when mseal was introduced in 6.10, semantically, when a VMA
> >> within the specified address range is sealed, the mprotect will be rejected,
> >> leaving all of VMA unmodified. However, adding an extra loop to check the mseal
> >> flag for every VMA slows things down a bit, therefore in 6.12, this issue was
> >> solved by removing can_modify_mm and checking each VMA’s mseal flag directly
> >> without an extra loop [1]. This is a semantic change, i.e. partial update is
> >> allowed, VMAs can be updated until a sealed VMA is found.
> >>
> >> The new semantic also means, we could allow mprotect on a sealed VMA if the new
> >> attribute of VMA remains the same as the old one. Relaxing this avoids unnecessary
> >> impacts for applications that want to seal a particular mapping. Doing this also
> >> has no security impact.
> >>
> >> [1] https://lore.kernel.org/all/20240817-mseal-depessimize-v3-0-d8d2e037df30@gmail.com/
> >>
> >> Fixes: 4a2dd02b0916 ("mm/mprotect: replace can_modify_mm with can_modify_vma")
> >> Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> >> ---
> >>  mm/mprotect.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/mm/mprotect.c b/mm/mprotect.c
> >> index 516b1d847e2c..a24d23967aa5 100644
> >> --- a/mm/mprotect.c
> >> +++ b/mm/mprotect.c
> >> @@ -613,14 +613,14 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
> >>      unsigned long charged = 0;
> >>      int error;
> >>
> >> -    if (!can_modify_vma(vma))
> >> -            return -EPERM;
> >> -
> >>      if (newflags == oldflags) {
> >>              *pprev = vma;
> >>              return 0;
> >>      }
> >>
> >> +    if (!can_modify_vma(vma))
> >> +            return -EPERM;
> >> +
> >>      /*
> >>       * Do PROT_NONE PFN permission checks here when we can still
> >>       * bail out without undoing a lot of state. This is a rather
> >> --
> >> 2.49.0.rc0.332.g42c0ae87b1-goog
> >>
> >
> >Hm I'm not so sure about this, to me a seal means 'don't touch', even if
> >the touch would be a no-op. It's simpler to be totally consistent on this
> >and makes the code easier everywhere.
> >
> >Because if we start saying 'apply mseal rules, except if we can determine
> >this to be a no-op' then that implies we might have some inconsistency in
> >other operations that do not do that, and sometimes a 'no-op' might be
> >ill-defined etc.
>
> Does mseal mean "you cannot call mprotect on this VMA" or does it mean "you cannot change this VMA". I've always considered it the latter since the entry point to making VMA changes doesn't matter (mmap, mprotect, etc) it's the VMA that can't change. Even the internal function name is "can_modify", and if the flags aren't changing then it's not a modification.
>
> I think it's more ergonomic to check for _changes_.

I think this is a slippery slope because some changes are not trivial
to deal with e.g
int fd = open("somefile")
void *ptr = mmap(0, 4096, PROT_READ, MAP_SHARED, fd, 0);
mmap(ptr, 4096, PROT_READ, MAP_FIXED | MAP_SHARED, fd, 0);


soooo on one hand, I don't really have grounds to say this patch is
incorrect. On the other hand, I'd like to see either a particular
problem or a consistent criteria we can apply to all VMA-related
situations.

-- 
Pedro