On Wed, Mar 12, 2025 at 12:21:17AM +0000, jeffxu@xxxxxxxxxxxx wrote: > From: Jeff Xu <jeffxu@xxxxxxxxxxxx> > > 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@xxxxxxxxx/ > > Fixes: 4a2dd02b0916 ("mm/mprotect: replace can_modify_mm with can_modify_vma") > Signed-off-by: Jeff Xu <jeffxu@xxxxxxxxxxxx> > --- > 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. I think generally I'd rather leave things as they are unless you have a specific real-life case where this is causing problems?
