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: <c867bff9-2293-4890-af46-8a797cf512c2@lucifer.local>
Date: Wed, 12 Mar 2025 13:49:39 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: jeffxu@...omium.org
Cc: 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, pedro.falcato@...il.com,
        rdunlap@...radead.org, jannh@...gle.com
Subject: Re: [RFC PATCH v1 2/2] mseal: allow noop mprotect

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.

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