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

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

[jeffxu]

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.

The mseal_test is also modified by this patch to adapt to the new
semantic. Please note, mseal_test is currently undergoing refactoring,
and eventually will be replaced with a new memory sealing selftest.
In this patch, I only make a minimum change to make it pass. I considered
adding a new testcase in mseal_test to cover this new behavior, however, the
existing mseal_test is using wrong patterns and won’t pass the review.
Such a new test is better to be added in the new refactored memory sealing tests.
The refactoring is currently pending review [2].

[1] https://lore.kernel.org/all/20240817-mseal-depessimize-v3-0-d8d2e037df30@gmail.com/
[2] https://lore.kernel.org/all/20241211053311.245636-1-jeffxu@google.com/

Jeff Xu (2):
  selftests/mm: mseal_test: avoid using no-op mprotect
  mseal: allow noop mprotect

 mm/mprotect.c                           | 6 +++---
 tools/testing/selftests/mm/mseal_test.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

[RFC PATCH v1 1/2] selftests/mm: mseal_test: avoid using no-op mprotect

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Modify mseal_tests to avoid using no-op mprotect.
The no-op mprotect shall be allowed.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Fixes: 4a2dd02b0916 ("mm/mprotect: replace can_modify_mm with can_modify_vma")
---
 tools/testing/selftests/mm/mseal_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index ad17005521a8..0d4e5d8aeefb 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -677,7 +677,7 @@ static void test_seal_mprotect_two_vma(bool seal)
 		FAIL_TEST_IF_FALSE(!ret);
 	}
 
-	ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
+	ret = sys_mprotect(ptr, page_size * 2, PROT_READ);
 	if (seal)
 		FAIL_TEST_IF_FALSE(ret < 0);
 	else
@@ -718,7 +718,7 @@ static void test_seal_mprotect_two_vma_with_split(bool seal)
 	FAIL_TEST_IF_FALSE(!ret);
 
 	/* the second page is sealed. */
-	ret = sys_mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
+	ret = sys_mprotect(ptr + page_size, page_size, PROT_READ);
 	if (seal)
 		FAIL_TEST_IF_FALSE(ret < 0);
 	else
@@ -873,7 +873,7 @@ static void test_seal_mprotect_split(bool seal)
 		FAIL_TEST_IF_FALSE(!ret);
 
 
-	ret = sys_mprotect(ptr + 2 * page_size, 2 * page_size, PROT_READ);
+	ret = sys_mprotect(ptr + 2 * page_size, 2 * page_size, PROT_WRITE);
 	if (seal)
 		FAIL_TEST_IF_FALSE(ret < 0);
 	else
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

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

[jeffxu]

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

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

[Lorenzo Stoakes]

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?

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

[Kees Cook]

On March 12, 2025 6:49:39 AM PDT, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>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.

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_.

-Kees

-- 
Kees Cook

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

[Pedro Falcato]

On Wed, Mar 12, 2025 at 3:28 PM Kees Cook <kees@kernel.org> wrote:
>
>
>
> On March 12, 2025 6:49:39 AM PDT, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >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.
>
> 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

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

[Lorenzo Stoakes]

On Wed, Mar 12, 2025 at 08:27:57AM -0700, Kees Cook wrote:
>
>
> On March 12, 2025 6:49:39 AM PDT, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >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.
>
> 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.

Right, but here it's easy to determine that.

What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
That's a no-op right? But it's not permitted.

So now we have an inconsistency between the two calls.

Should we now check to see if all the madvise() calls are somehow no-ops
and permit them? Because that gets potentially egregious, fast.

My concern is that we set a trap for ourselves by establishing some kind of
contract, implicit or not, that otherwise-mseal-prevented-calls will be
permitted if they result in a no-op.

To me it's simpler to say 'if we touch a VMA with a call that modifies
things, and it's sealed, we abort'.

Easy, doesn't set traps, no reasonable situation in which that should cause
problems.

>
> I think it's more ergonomic to check for _changes_.

I don't know what you mean by 'ergonomic'?

>
> -Kees
>
> --
> Kees Cook

My reply seemed to get truncated at the end here :) So let me ask again -
do you have a practical case in mind for this?

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

[Kees Cook]

On Wed, Mar 12, 2025 at 03:50:40PM +0000, Lorenzo Stoakes wrote:
> What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
> That's a no-op right? But it's not permitted.

Hmm, yes, that's a good example. Thank you!

> So now we have an inconsistency between the two calls.

Yeah, I see your concern now.

> I don't know what you mean by 'ergonomic'?

I was thinking about idempotent-ness. Like, some library setting up a
memory region, it can't call its setup routine twice if the second time
through (where no changes are made) it gets rejected. But I think this
is likely just a userspace problem: check for the VMAs before blindly
trying to do it again. (This is strictly an imagined situation.)

> My reply seemed to get truncated at the end here :) So let me ask again -
> do you have a practical case in mind for this?

Sorry, I didn't have any reply to that part, so I left it off. If Jeff
has a specific case in mind, I'll let him answer that part. :)

-Kees

-- 
Kees Cook

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

[Jeff Xu]

On Wed, Mar 12, 2025 at 9:45 AM Kees Cook <kees@kernel.org> wrote:
>
> On Wed, Mar 12, 2025 at 03:50:40PM +0000, Lorenzo Stoakes wrote:
> > What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
> > That's a no-op right? But it's not permitted.
>
Madvise's semantics are about behavior, while mprotect is about
attributes. To me:  madvise is like "make this VMA do that" while
mprotect is about "update this VMA's attributes to a new value".

It is more difficult to determine if a behavior is no-op, so I don't
intend to apply the same no-op concept to madvise().

> Hmm, yes, that's a good example. Thank you!
>
> > So now we have an inconsistency between the two calls.
>
> Yeah, I see your concern now.
>
> > I don't know what you mean by 'ergonomic'?
>
> I was thinking about idempotent-ness. Like, some library setting up a
> memory region, it can't call its setup routine twice if the second time
> through (where no changes are made) it gets rejected. But I think this
> is likely just a userspace problem: check for the VMAs before blindly
> trying to do it again. (This is strictly an imagined situation.)
>
Yes.

 We also don't have a system call to query the "mprotect" attributes,
so it is understandable that userspace can rely on idempotents of the
mprotect.

> > My reply seemed to get truncated at the end here :) So let me ask again -
> > do you have a practical case in mind for this?
>
I noticed there were idempotent mprotects last year while working on
applying mseal on stack in Android. I assume this might not be the
only instance since mprotect gets called a lot in general.

Blocking this won't improve security, it could actually hinder the
adoption of mseal, i.e. force apps to make code change.

-Jeff

> Sorry, I didn't have any reply to that part, so I left it off. If Jeff
> has a specific case in mind, I'll let him answer that part. :)
>
> -Kees
>
> --
> Kees Cook

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

[Lorenzo Stoakes]

On Wed, Mar 12, 2025 at 04:29:50PM -0700, Jeff Xu wrote:
> On Wed, Mar 12, 2025 at 9:45 AM Kees Cook <kees@kernel.org> wrote:
> >
> > On Wed, Mar 12, 2025 at 03:50:40PM +0000, Lorenzo Stoakes wrote:
> > > What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
> > > That's a no-op right? But it's not permitted.
> >
> Madvise's semantics are about behavior, while mprotect is about
> attributes. To me:  madvise is like "make this VMA do that" while
> mprotect is about "update this VMA's attributes to a new value".
>
> It is more difficult to determine if a behavior is no-op, so I don't
> intend to apply the same no-op concept to madvise().
>
> > Hmm, yes, that's a good example. Thank you!
> >
> > > So now we have an inconsistency between the two calls.
> >
> > Yeah, I see your concern now.
> >
> > > I don't know what you mean by 'ergonomic'?
> >
> > I was thinking about idempotent-ness. Like, some library setting up a
> > memory region, it can't call its setup routine twice if the second time
> > through (where no changes are made) it gets rejected. But I think this
> > is likely just a userspace problem: check for the VMAs before blindly
> > trying to do it again. (This is strictly an imagined situation.)
> >
> Yes.
>
>  We also don't have a system call to query the "mprotect" attributes,
> so it is understandable that userspace can rely on idempotents of the
> mprotect.

PROCMAP_QUERY ioctl, /proc/$pid/pagemap :) I mean hey - these are somewhat
diagnostic-y, racey, un-fun interfaces that we'd rather you not use in
anger when mapping stuff - but they do at least exist :)

(an aside, been playing with PROCMAP_QUERY recently and very cool - we plan
to make this useable under RCU lock rather than mmap lock which will make
it _even more_ useful in future... exciting times :)

It's possible, but it seems that it would be relying upon it purely because
in some cases it would be modifying the mapping, right?

It strikes me as very unlikely that an application would be looking to
modify the attributes of a series of VMAs including ones that have a
security feature enabled which says 'until this is unmapped do not modify
the attributes of this VMA'.

Yes it's _theoretically_ possible but that'd be quite silly no?

>
> > > My reply seemed to get truncated at the end here :) So let me ask again -
> > > do you have a practical case in mind for this?
> >
> I noticed there were idempotent mprotects last year while working on
> applying mseal on stack in Android. I assume this might not be the
> only instance since mprotect gets called a lot in general.
>
> Blocking this won't improve security, it could actually hinder the
> adoption of mseal, i.e. force apps to make code change.

Thanks for the explanation it's appreciated!

But I feel the drawbacks I mentioned previously and elucidated upon in my
reply to Kees outweigh this theoretical concern.

If we encounter actual real-world instances of this we can reconsider,
presuming we are ok with the asymmetry vs. other seal-protected calls. We
have this shipped with a uAPI already like this so there's no rush.

>
> -Jeff
>
> > Sorry, I didn't have any reply to that part, so I left it off. If Jeff
> > has a specific case in mind, I'll let him answer that part. :)
> >
> > -Kees
> >
> > --
> > Kees Cook

Cheers, Lorenzo

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

[Jeff Xu]

On Wed, Mar 12, 2025 at 10:29 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Mar 12, 2025 at 04:29:50PM -0700, Jeff Xu wrote:
> > On Wed, Mar 12, 2025 at 9:45 AM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Wed, Mar 12, 2025 at 03:50:40PM +0000, Lorenzo Stoakes wrote:
> > > > What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
> > > > That's a no-op right? But it's not permitted.
> > >
> > Madvise's semantics are about behavior, while mprotect is about
> > attributes. To me:  madvise is like "make this VMA do that" while
> > mprotect is about "update this VMA's attributes to a new value".
> >
> > It is more difficult to determine if a behavior is no-op, so I don't
> > intend to apply the same no-op concept to madvise().
> >
> > > Hmm, yes, that's a good example. Thank you!
> > >
> > > > So now we have an inconsistency between the two calls.
> > >
> > > Yeah, I see your concern now.
> > >
> > > > I don't know what you mean by 'ergonomic'?
> > >
> > > I was thinking about idempotent-ness. Like, some library setting up a
> > > memory region, it can't call its setup routine twice if the second time
> > > through (where no changes are made) it gets rejected. But I think this
> > > is likely just a userspace problem: check for the VMAs before blindly
> > > trying to do it again. (This is strictly an imagined situation.)
> > >
> > Yes.
> >
> >  We also don't have a system call to query the "mprotect" attributes,
> > so it is understandable that userspace can rely on idempotents of the
> > mprotect.
>
> PROCMAP_QUERY ioctl, /proc/$pid/pagemap :) I mean hey - these are somewhat
> diagnostic-y, racey, un-fun interfaces that we'd rather you not use in
> anger when mapping stuff - but they do at least exist :)
>
> (an aside, been playing with PROCMAP_QUERY recently and very cool - we plan
> to make this useable under RCU lock rather than mmap lock which will make
> it _even more_ useful in future... exciting times :)
>
/proc/pid/maps only has a subset information of vm_flags, e.g. pkeys
is not part of it, however pkey_mprotect can update pkey. So the
suggestion of checking for the VMAs before calling mprotect won't work
for all cases. Besides,  the checking then updating pattern also has
the perf impact due to an extra syscall.

> > > trying to do it again. (

> It's possible, but it seems that it would be relying upon it purely because
> in some cases it would be modifying the mapping, right?
>
> It strikes me as very unlikely that an application would be looking to
> modify the attributes of a series of VMAs including ones that have a
> security feature enabled which says 'until this is unmapped do not modify
> the attributes of this VMA'.
>
> Yes it's _theoretically_ possible but that'd be quite silly no?
>
> >
> > > > My reply seemed to get truncated at the end here :) So let me ask again -
> > > > do you have a practical case in mind for this?
> > >
> > I noticed there were idempotent mprotects last year while working on
> > applying mseal on stack in Android. I assume this might not be the
> > only instance since mprotect gets called a lot in general.
> >
> > Blocking this won't improve security, it could actually hinder the
> > adoption of mseal, i.e. force apps to make code change.
>
> Thanks for the explanation it's appreciated!
>
> But I feel the drawbacks I mentioned previously and elucidated upon in my
> reply to Kees outweigh this theoretical concern.
>
> If we encounter actual real-world instances of this we can reconsider,
> presuming we are ok with the asymmetry vs. other seal-protected calls. We
> have this shipped with a uAPI already like this so there's no rush.
>
Sure. But I honestly think that you are overthinking on this. The
security benefit of mseal for pkey_mprotect is that an attacker can't
modify the VMA's attributes,  and this patch does not compromise on
that.

Best regards,
-Jeff


> >
> > -Jeff
> >
> > > Sorry, I didn't have any reply to that part, so I left it off. If Jeff
> > > has a specific case in mind, I'll let him answer that part. :)
> > >
> > > -Kees
> > >
> > > --
> > > Kees Cook
>
> Cheers, Lorenzo