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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Mar 12, 2025 at 12:21:17AM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@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-d8d2e037df30@gmail.com/
>
> Fixes: 4a2dd02b0916 ("mm/mprotect: replace can_modify_mm with can_modify_vma")
> Signed-off-by: Jeff Xu <jeffxu@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.

I think generally I'd rather leave things as they are unless you have a
specific real-life case where this is causing problems?