linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/mremap: fix regression in vrm->new_addr check
@ 2025-08-28  3:26 Carlos Llamas
  2025-08-28  3:43 ` Liam R. Howlett
  2025-08-28  5:38 ` Lorenzo Stoakes
  0 siblings, 2 replies; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28  3:26 UTC (permalink / raw)
  To: Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Vlastimil Babka,
	Jann Horn, Pedro Falcato
  Cc: kernel-team, linux-kernel, Carlos Llamas,
	open list:MEMORY MAPPING

Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
checks") moved the sanity check for vrm->new_addr from mremap_to() to
check_mremap_params().

However, this caused a regression as vrm->new_addr is now checked even
when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
case, vrm->new_addr can be garbage and create unexpected failures.

Fix this by moving the new_addr check after the vrm_implies_new_addr()
guard. This ensures that the new_addr is only checked when the user has
specified one explicitly.

Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 mm/mremap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index e618a706aff5..692acb0f9ea2 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1771,18 +1771,17 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
 	 * for DOS-emu "duplicate shm area" thing. But
 	 * a zero new-len is nonsensical.
 	 */
-	if (!vrm->new_len)
-		return -EINVAL;
-
-	/* Is the new length or address silly? */
-	if (vrm->new_len > TASK_SIZE ||
-	    vrm->new_addr > TASK_SIZE - vrm->new_len)
+	if (!vrm->new_len || vrm->new_len > TASK_SIZE)
 		return -EINVAL;
 
 	/* Remainder of checks are for cases with specific new_addr. */
 	if (!vrm_implies_new_addr(vrm))
 		return 0;
 
+	/* Is the new address silly? */
+	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
+		return -EINVAL;
+
 	/* The new address must be page-aligned. */
 	if (offset_in_page(vrm->new_addr))
 		return -EINVAL;
-- 
2.51.0.268.g9569e192d0-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  3:26 [PATCH] mm/mremap: fix regression in vrm->new_addr check Carlos Llamas
@ 2025-08-28  3:43 ` Liam R. Howlett
  2025-08-28  4:06   ` Carlos Llamas
  2025-08-28  5:38 ` Lorenzo Stoakes
  1 sibling, 1 reply; 13+ messages in thread
From: Liam R. Howlett @ 2025-08-28  3:43 UTC (permalink / raw)
  To: Carlos Llamas
  Cc: Andrew Morton, Lorenzo Stoakes, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

* Carlos Llamas <cmllamas@google.com> [250827 23:27]:
> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> check_mremap_params().
> 
> However, this caused a regression as vrm->new_addr is now checked even
> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> case, vrm->new_addr can be garbage and create unexpected failures.
> 
> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> guard. This ensures that the new_addr is only checked when the user has
> specified one explicitly.
> 
> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

I assume this showed up with clang?

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/mremap.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e618a706aff5..692acb0f9ea2 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1771,18 +1771,17 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
>  	 * for DOS-emu "duplicate shm area" thing. But
>  	 * a zero new-len is nonsensical.
>  	 */
> -	if (!vrm->new_len)
> -		return -EINVAL;
> -
> -	/* Is the new length or address silly? */
> -	if (vrm->new_len > TASK_SIZE ||
> -	    vrm->new_addr > TASK_SIZE - vrm->new_len)
> +	if (!vrm->new_len || vrm->new_len > TASK_SIZE)
>  		return -EINVAL;
>  
>  	/* Remainder of checks are for cases with specific new_addr. */
>  	if (!vrm_implies_new_addr(vrm))
>  		return 0;
>  
> +	/* Is the new address silly? */
> +	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
> +		return -EINVAL;
> +
>  	/* The new address must be page-aligned. */
>  	if (offset_in_page(vrm->new_addr))
>  		return -EINVAL;
> -- 
> 2.51.0.268.g9569e192d0-goog
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  3:43 ` Liam R. Howlett
@ 2025-08-28  4:06   ` Carlos Llamas
  2025-08-28  4:16     ` Carlos Llamas
  0 siblings, 1 reply; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28  4:06 UTC (permalink / raw)
  To: Liam R. Howlett
  Cc: Andrew Morton, Lorenzo Stoakes, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Wed, Aug 27, 2025 at 11:43:39PM -0400, Liam R. Howlett wrote:
> * Carlos Llamas <cmllamas@google.com> [250827 23:27]:
> > Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> > checks") moved the sanity check for vrm->new_addr from mremap_to() to
> > check_mremap_params().
> > 
> > However, this caused a regression as vrm->new_addr is now checked even
> > when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> > case, vrm->new_addr can be garbage and create unexpected failures.
> > 
> > Fix this by moving the new_addr check after the vrm_implies_new_addr()
> > guard. This ensures that the new_addr is only checked when the user has
> > specified one explicitly.
> > 
> > Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> I assume this showed up with clang?

Right.

The specific test that broke on our end was this:
https://android.googlesource.com/platform/bionic/+/HEAD/tests/__cxa_atexit_test.cpp
Although I'm not exactly sure how __cxa_atexit() implementation uses
mremap() underneath.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  4:06   ` Carlos Llamas
@ 2025-08-28  4:16     ` Carlos Llamas
  0 siblings, 0 replies; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28  4:16 UTC (permalink / raw)
  To: Liam R. Howlett
  Cc: Andrew Morton, Lorenzo Stoakes, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Thu, Aug 28, 2025 at 04:06:58AM +0000, Carlos Llamas wrote:
> On Wed, Aug 27, 2025 at 11:43:39PM -0400, Liam R. Howlett wrote:
> > * Carlos Llamas <cmllamas@google.com> [250827 23:27]:
> > > Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> > > checks") moved the sanity check for vrm->new_addr from mremap_to() to
> > > check_mremap_params().
> > > 
> > > However, this caused a regression as vrm->new_addr is now checked even
> > > when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> > > case, vrm->new_addr can be garbage and create unexpected failures.
> > > 
> > > Fix this by moving the new_addr check after the vrm_implies_new_addr()
> > > guard. This ensures that the new_addr is only checked when the user has
> > > specified one explicitly.
> > > 
> > > Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > 
> > I assume this showed up with clang?
> 
> Right.
> 
> The specific test that broke on our end was this:
> https://android.googlesource.com/platform/bionic/+/HEAD/tests/__cxa_atexit_test.cpp
> Although I'm not exactly sure how __cxa_atexit() implementation uses
> mremap() underneath.

Ok, here is the specific call to mremap() in the test:
https://android.googlesource.com/platform/bionic/+/HEAD/libc/bionic/atexit.cpp#197

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  3:26 [PATCH] mm/mremap: fix regression in vrm->new_addr check Carlos Llamas
  2025-08-28  3:43 ` Liam R. Howlett
@ 2025-08-28  5:38 ` Lorenzo Stoakes
  2025-08-28 14:13   ` Carlos Llamas
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Lorenzo Stoakes @ 2025-08-28  5:38 UTC (permalink / raw)
  To: Carlos Llamas
  Cc: Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Thu, Aug 28, 2025 at 03:26:52AM +0000, Carlos Llamas wrote:
> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> check_mremap_params().
>
> However, this caused a regression as vrm->new_addr is now checked even
> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> case, vrm->new_addr can be garbage and create unexpected failures.

Yikes, sorry my mistake.

>
> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> guard. This ensures that the new_addr is only checked when the user has
> specified one explicitly.
>
> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

You need a Cc: Stable.

> ---
>  mm/mremap.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e618a706aff5..692acb0f9ea2 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1771,18 +1771,17 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
>  	 * for DOS-emu "duplicate shm area" thing. But
>  	 * a zero new-len is nonsensical.
>  	 */
> -	if (!vrm->new_len)
> -		return -EINVAL;

Please don't refactor code at the same time, especially as this is a hotfix.

This line is associated with the above comment.

> -
> -	/* Is the new length or address silly? */
> -	if (vrm->new_len > TASK_SIZE ||
> -	    vrm->new_addr > TASK_SIZE - vrm->new_len)
> +	if (!vrm->new_len || vrm->new_len > TASK_SIZE)
>  		return -EINVAL;

Yeah, it's cute but by removing the 'silly' comment you're making this
associated with the comment above and... yeah.

This should be:

	/*
	 * We allow a zero old-len as a special case
	 * for DOS-emu "duplicate shm area" thing. But
	 * a zero new-len is nonsensical.
	 */
	if (!vrm->new_len)
		return -EINVAL;

	/* Is the new length silly? */
	if (vrm->new_len > TASK_SIZE)
		return -EINVAL;

>
>  	/* Remainder of checks are for cases with specific new_addr. */
>  	if (!vrm_implies_new_addr(vrm))
>  		return 0;
>
> +	/* Is the new address silly? */
> +	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
> +		return -EINVAL;
> +

Obviously this bit is fine :)

Sorry again, this was just a mistake on my part.

>  	/* The new address must be page-aligned. */
>  	if (offset_in_page(vrm->new_addr))
>  		return -EINVAL;
> --
> 2.51.0.268.g9569e192d0-goog
>

I'm curious why only you guys have seen it, a theory is bionic is sending random
stuff to this parameter when unspecified, and glibc is not.

But obviously this fix is correct, and the original code needs fixing.

Please respin a v2 as per above.

Cheers, Lorenzo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  5:38 ` Lorenzo Stoakes
@ 2025-08-28 14:13   ` Carlos Llamas
  2025-08-28 14:21   ` Vlastimil Babka
  2025-08-28 14:26   ` [PATCH v2] " Carlos Llamas
  2 siblings, 0 replies; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28 14:13 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Thu, Aug 28, 2025 at 06:38:09AM +0100, Lorenzo Stoakes wrote:
> I'm curious why only you guys have seen it, a theory is bionic is sending random
> stuff to this parameter when unspecified, and glibc is not.

I honestly don't know and it could very well be only with bionic or
clang. I'll try to find out more about this, maybe glibc implementation
zeros out the 'new_addres' register and bionic doesn't.

> 
> But obviously this fix is correct, and the original code needs fixing.
> 
> Please respin a v2 as per above.

Sounds good.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  5:38 ` Lorenzo Stoakes
  2025-08-28 14:13   ` Carlos Llamas
@ 2025-08-28 14:21   ` Vlastimil Babka
  2025-08-28 14:22     ` Lorenzo Stoakes
  2025-08-28 14:26   ` [PATCH v2] " Carlos Llamas
  2 siblings, 1 reply; 13+ messages in thread
From: Vlastimil Babka @ 2025-08-28 14:21 UTC (permalink / raw)
  To: Lorenzo Stoakes, Carlos Llamas
  Cc: Andrew Morton, Liam R. Howlett, Jann Horn, Pedro Falcato,
	kernel-team, linux-kernel, open list:MEMORY MAPPING

On 8/28/25 07:38, Lorenzo Stoakes wrote:
> On Thu, Aug 28, 2025 at 03:26:52AM +0000, Carlos Llamas wrote:
>> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
>> checks") moved the sanity check for vrm->new_addr from mremap_to() to
>> check_mremap_params().
>>
>> However, this caused a regression as vrm->new_addr is now checked even
>> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
>> case, vrm->new_addr can be garbage and create unexpected failures.
> 
> Yikes, sorry my mistake.
> 
>>
>> Fix this by moving the new_addr check after the vrm_implies_new_addr()
>> guard. This ensures that the new_addr is only checked when the user has
>> specified one explicitly.
>>
>> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
>> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> You need a Cc: Stable.

No need as the commit being fixed is from 6.17-rc1?
But it's better to use "[PATCH mm-hotfixes]" to make sure it goes to 6.17
and not the next merge window.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28 14:21   ` Vlastimil Babka
@ 2025-08-28 14:22     ` Lorenzo Stoakes
  2025-08-28 14:29       ` Carlos Llamas
  0 siblings, 1 reply; 13+ messages in thread
From: Lorenzo Stoakes @ 2025-08-28 14:22 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Carlos Llamas, Andrew Morton, Liam R. Howlett, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Thu, Aug 28, 2025 at 04:21:05PM +0200, Vlastimil Babka wrote:
> On 8/28/25 07:38, Lorenzo Stoakes wrote:
> > On Thu, Aug 28, 2025 at 03:26:52AM +0000, Carlos Llamas wrote:
> >> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> >> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> >> check_mremap_params().
> >>
> >> However, this caused a regression as vrm->new_addr is now checked even
> >> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> >> case, vrm->new_addr can be garbage and create unexpected failures.
> >
> > Yikes, sorry my mistake.
> >
> >>
> >> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> >> guard. This ensures that the new_addr is only checked when the user has
> >> specified one explicitly.
> >>
> >> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> >> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> >
> > You need a Cc: Stable.
>
> No need as the commit being fixed is from 6.17-rc1?
> But it's better to use "[PATCH mm-hotfixes]" to make sure it goes to 6.17
> and not the next merge window.
>

Ah haha really? I'm losing track of my patches.

Yeah sure as per Vlasta then Carlos :)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28  5:38 ` Lorenzo Stoakes
  2025-08-28 14:13   ` Carlos Llamas
  2025-08-28 14:21   ` Vlastimil Babka
@ 2025-08-28 14:26   ` Carlos Llamas
  2025-08-28 14:30     ` Vlastimil Babka
  2025-08-28 14:34     ` Lorenzo Stoakes
  2 siblings, 2 replies; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28 14:26 UTC (permalink / raw)
  To: Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Vlastimil Babka,
	Jann Horn, Pedro Falcato
  Cc: kernel-team, linux-kernel, Carlos Llamas, stable,
	open list:MEMORY MAPPING

Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
checks") moved the sanity check for vrm->new_addr from mremap_to() to
check_mremap_params().

However, this caused a regression as vrm->new_addr is now checked even
when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
case, vrm->new_addr can be garbage and create unexpected failures.

Fix this by moving the new_addr check after the vrm_implies_new_addr()
guard. This ensures that the new_addr is only checked when the user has
specified one explicitly.

Cc: stable@vger.kernel.org
Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
v2:
 - split out vrm->new_len into individual checks
 - cc stable, collect tags

v1:
https://lore.kernel.org/all/20250828032653.521314-1-cmllamas@google.com/

 mm/mremap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index e618a706aff5..35de0a7b910e 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1774,15 +1774,18 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
 	if (!vrm->new_len)
 		return -EINVAL;
 
-	/* Is the new length or address silly? */
-	if (vrm->new_len > TASK_SIZE ||
-	    vrm->new_addr > TASK_SIZE - vrm->new_len)
+	/* Is the new length silly? */
+	if (vrm->new_len > TASK_SIZE)
 		return -EINVAL;
 
 	/* Remainder of checks are for cases with specific new_addr. */
 	if (!vrm_implies_new_addr(vrm))
 		return 0;
 
+	/* Is the new address silly? */
+	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
+		return -EINVAL;
+
 	/* The new address must be page-aligned. */
 	if (offset_in_page(vrm->new_addr))
 		return -EINVAL;
-- 
2.51.0.268.g9569e192d0-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28 14:22     ` Lorenzo Stoakes
@ 2025-08-28 14:29       ` Carlos Llamas
  2025-08-28 14:31         ` Vlastimil Babka
  0 siblings, 1 reply; 13+ messages in thread
From: Carlos Llamas @ 2025-08-28 14:29 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: Vlastimil Babka, Andrew Morton, Liam R. Howlett, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel,
	open list:MEMORY MAPPING

On Thu, Aug 28, 2025 at 03:22:54PM +0100, Lorenzo Stoakes wrote:
> On Thu, Aug 28, 2025 at 04:21:05PM +0200, Vlastimil Babka wrote:
> > On 8/28/25 07:38, Lorenzo Stoakes wrote:
> > > On Thu, Aug 28, 2025 at 03:26:52AM +0000, Carlos Llamas wrote:
> > >> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> > >> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> > >> check_mremap_params().
> > >>
> > >> However, this caused a regression as vrm->new_addr is now checked even
> > >> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> > >> case, vrm->new_addr can be garbage and create unexpected failures.
> > >
> > > Yikes, sorry my mistake.
> > >
> > >>
> > >> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> > >> guard. This ensures that the new_addr is only checked when the user has
> > >> specified one explicitly.
> > >>
> > >> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> > >> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > >
> > > You need a Cc: Stable.
> >
> > No need as the commit being fixed is from 6.17-rc1?
> > But it's better to use "[PATCH mm-hotfixes]" to make sure it goes to 6.17
> > and not the next merge window.
> >
> 
> Ah haha really? I'm losing track of my patches.
> 
> Yeah sure as per Vlasta then Carlos :)

Oops, sorry my v2 raced with this.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28 14:26   ` [PATCH v2] " Carlos Llamas
@ 2025-08-28 14:30     ` Vlastimil Babka
  2025-08-28 14:34     ` Lorenzo Stoakes
  1 sibling, 0 replies; 13+ messages in thread
From: Vlastimil Babka @ 2025-08-28 14:30 UTC (permalink / raw)
  To: Carlos Llamas, Andrew Morton, Liam R. Howlett, Lorenzo Stoakes,
	Jann Horn, Pedro Falcato
  Cc: kernel-team, linux-kernel, stable, open list:MEMORY MAPPING

On 8/28/25 16:26, Carlos Llamas wrote:
> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> check_mremap_params().
> 
> However, this caused a regression as vrm->new_addr is now checked even
> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> case, vrm->new_addr can be garbage and create unexpected failures.
> 
> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> guard. This ensures that the new_addr is only checked when the user has
> specified one explicitly.
> 
> Cc: stable@vger.kernel.org

Not necessary, but for mm-hotfixes please, Andrew.

> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> v2:
>  - split out vrm->new_len into individual checks
>  - cc stable, collect tags
> 
> v1:
> https://lore.kernel.org/all/20250828032653.521314-1-cmllamas@google.com/
> 
>  mm/mremap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e618a706aff5..35de0a7b910e 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1774,15 +1774,18 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
>  	if (!vrm->new_len)
>  		return -EINVAL;
>  
> -	/* Is the new length or address silly? */
> -	if (vrm->new_len > TASK_SIZE ||
> -	    vrm->new_addr > TASK_SIZE - vrm->new_len)
> +	/* Is the new length silly? */
> +	if (vrm->new_len > TASK_SIZE)
>  		return -EINVAL;
>  
>  	/* Remainder of checks are for cases with specific new_addr. */
>  	if (!vrm_implies_new_addr(vrm))
>  		return 0;
>  
> +	/* Is the new address silly? */
> +	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
> +		return -EINVAL;
> +
>  	/* The new address must be page-aligned. */
>  	if (offset_in_page(vrm->new_addr))
>  		return -EINVAL;


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28 14:29       ` Carlos Llamas
@ 2025-08-28 14:31         ` Vlastimil Babka
  0 siblings, 0 replies; 13+ messages in thread
From: Vlastimil Babka @ 2025-08-28 14:31 UTC (permalink / raw)
  To: Carlos Llamas, Lorenzo Stoakes
  Cc: Andrew Morton, Liam R. Howlett, Jann Horn, Pedro Falcato,
	kernel-team, linux-kernel, open list:MEMORY MAPPING

On 8/28/25 16:29, Carlos Llamas wrote:
> On Thu, Aug 28, 2025 at 03:22:54PM +0100, Lorenzo Stoakes wrote:
>> On Thu, Aug 28, 2025 at 04:21:05PM +0200, Vlastimil Babka wrote:
>> > On 8/28/25 07:38, Lorenzo Stoakes wrote:
>> > > On Thu, Aug 28, 2025 at 03:26:52AM +0000, Carlos Llamas wrote:
>> > >> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
>> > >> checks") moved the sanity check for vrm->new_addr from mremap_to() to
>> > >> check_mremap_params().
>> > >>
>> > >> However, this caused a regression as vrm->new_addr is now checked even
>> > >> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
>> > >> case, vrm->new_addr can be garbage and create unexpected failures.
>> > >
>> > > Yikes, sorry my mistake.
>> > >
>> > >>
>> > >> Fix this by moving the new_addr check after the vrm_implies_new_addr()
>> > >> guard. This ensures that the new_addr is only checked when the user has
>> > >> specified one explicitly.
>> > >>
>> > >> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
>> > >> Signed-off-by: Carlos Llamas <cmllamas@google.com>
>> > >
>> > > You need a Cc: Stable.
>> >
>> > No need as the commit being fixed is from 6.17-rc1?
>> > But it's better to use "[PATCH mm-hotfixes]" to make sure it goes to 6.17
>> > and not the next merge window.
>> >
>> 
>> Ah haha really? I'm losing track of my patches.
>> 
>> Yeah sure as per Vlasta then Carlos :)
> 
> Oops, sorry my v2 raced with this.

Nevermind, Andrew can fixup locally. Thanks :)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mm/mremap: fix regression in vrm->new_addr check
  2025-08-28 14:26   ` [PATCH v2] " Carlos Llamas
  2025-08-28 14:30     ` Vlastimil Babka
@ 2025-08-28 14:34     ` Lorenzo Stoakes
  1 sibling, 0 replies; 13+ messages in thread
From: Lorenzo Stoakes @ 2025-08-28 14:34 UTC (permalink / raw)
  To: Carlos Llamas
  Cc: Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
	Pedro Falcato, kernel-team, linux-kernel, stable,
	open list:MEMORY MAPPING

(For future reference) please send separately rather than in reply to first
:)

Otherwise harder for me to find your series!

On Thu, Aug 28, 2025 at 02:26:56PM +0000, Carlos Llamas wrote:
> Commit 3215eaceca87 ("mm/mremap: refactor initial parameter sanity
> checks") moved the sanity check for vrm->new_addr from mremap_to() to
> check_mremap_params().
>
> However, this caused a regression as vrm->new_addr is now checked even
> when MREMAP_FIXED and MREMAP_DONTUNMAP flags are not specified. In this
> case, vrm->new_addr can be garbage and create unexpected failures.
>
> Fix this by moving the new_addr check after the vrm_implies_new_addr()
> guard. This ensures that the new_addr is only checked when the user has
> specified one explicitly.
>
> Cc: stable@vger.kernel.org

Yeah oopsies on me suggesting this :P losing track of my own patches :)

> Fixes: 3215eaceca87 ("mm/mremap: refactor initial parameter sanity checks")
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

LGTM, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

And again thanks so much for this! :)

> ---
> v2:
>  - split out vrm->new_len into individual checks
>  - cc stable, collect tags
>
> v1:
> https://lore.kernel.org/all/20250828032653.521314-1-cmllamas@google.com/
>
>  mm/mremap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e618a706aff5..35de0a7b910e 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1774,15 +1774,18 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
>  	if (!vrm->new_len)
>  		return -EINVAL;
>
> -	/* Is the new length or address silly? */
> -	if (vrm->new_len > TASK_SIZE ||
> -	    vrm->new_addr > TASK_SIZE - vrm->new_len)
> +	/* Is the new length silly? */
> +	if (vrm->new_len > TASK_SIZE)
>  		return -EINVAL;
>
>  	/* Remainder of checks are for cases with specific new_addr. */
>  	if (!vrm_implies_new_addr(vrm))
>  		return 0;
>
> +	/* Is the new address silly? */
> +	if (vrm->new_addr > TASK_SIZE - vrm->new_len)
> +		return -EINVAL;
> +
>  	/* The new address must be page-aligned. */
>  	if (offset_in_page(vrm->new_addr))
>  		return -EINVAL;
> --
> 2.51.0.268.g9569e192d0-goog
>

Cheers, Lorenzo

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-08-28 14:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-28  3:26 [PATCH] mm/mremap: fix regression in vrm->new_addr check Carlos Llamas
2025-08-28  3:43 ` Liam R. Howlett
2025-08-28  4:06   ` Carlos Llamas
2025-08-28  4:16     ` Carlos Llamas
2025-08-28  5:38 ` Lorenzo Stoakes
2025-08-28 14:13   ` Carlos Llamas
2025-08-28 14:21   ` Vlastimil Babka
2025-08-28 14:22     ` Lorenzo Stoakes
2025-08-28 14:29       ` Carlos Llamas
2025-08-28 14:31         ` Vlastimil Babka
2025-08-28 14:26   ` [PATCH v2] " Carlos Llamas
2025-08-28 14:30     ` Vlastimil Babka
2025-08-28 14:34     ` Lorenzo Stoakes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).