* [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).