On Wed, Mar 12, 2025 at 3:28 PM Kees Cook <k...@kernel.org> wrote:
>
>
>
> On March 12, 2025 6:49:39 AM PDT, Lorenzo Stoakes
> <lorenzo.stoa...@oracle.com> wrote:
> >On Wed, Mar 12, 2025 at 12:21:17AM +0000, jef...@chromium.org wrote:
> >> From: Jeff Xu <jef...@chromium.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-d8d2e037d...@gmail.com/
> >>
> >> Fixes: 4a2dd02b0916 ("mm/mprotect: replace can_modify_mm with
> >> can_modify_vma")
> >> Signed-off-by: Jeff Xu <jef...@chromium.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
