[PATCH] arm64: Clear the initial ID map correctly before remapping

[PATCH] arm64: Clear the initial ID map correctly before remapping

[Zenghui Yu]

In the attempt to clear and recreate the initial ID map for LPA2, we
wrongly use 'start - end' as the map size and make the memset() almost a
nop.

Fix it by passing the correct map size.

Fixes: 9684ec186f8f ("arm64: Enable LPA2 at boot if supported by the system")
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---

Found by code inspection (don't have the appropriate HW to test it).

 arch/arm64/kernel/pi/map_kernel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
index 5fa08e13e17e..f374a3e5a5fe 100644
--- a/arch/arm64/kernel/pi/map_kernel.c
+++ b/arch/arm64/kernel/pi/map_kernel.c
@@ -173,7 +173,7 @@ static void __init remap_idmap_for_lpa2(void)
 	 * Don't bother with the FDT, we no longer need it after this.
 	 */
 	memset(init_idmap_pg_dir, 0,
-	       (u64)init_idmap_pg_dir - (u64)init_idmap_pg_end);
+	       (u64)init_idmap_pg_end - (u64)init_idmap_pg_dir);
 
 	create_init_idmap(init_idmap_pg_dir, mask);
 	dsb(ishst);
-- 
2.33.0

Re: [PATCH] arm64: Clear the initial ID map correctly before remapping

[Ard Biesheuvel]

On Fri, 21 Jun 2024 at 11:28, Zenghui Yu <yuzenghui@huawei.com> wrote:
>
> In the attempt to clear and recreate the initial ID map for LPA2, we
> wrongly use 'start - end' as the map size and make the memset() almost a
> nop.
>
> Fix it by passing the correct map size.
>
> Fixes: 9684ec186f8f ("arm64: Enable LPA2 at boot if supported by the system")
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>
> Found by code inspection (don't have the appropriate HW to test it).
>

Good catch!

Even though memset() takes an unsigned size_t, the zeroing path in
arm64's memset.S does a signed compare on the provided size, and will
zero at most 63 bytes if the size has the sign bit set. So in the end,
it does not clear anything. Note that in this particular case, that
doesn't actually matter - the memory is reused immediately to create
another copy of the ID map, and any unused regions containing garbage
will just be ignored.

Nonetheless,

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>




>  arch/arm64/kernel/pi/map_kernel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
> index 5fa08e13e17e..f374a3e5a5fe 100644
> --- a/arch/arm64/kernel/pi/map_kernel.c
> +++ b/arch/arm64/kernel/pi/map_kernel.c
> @@ -173,7 +173,7 @@ static void __init remap_idmap_for_lpa2(void)
>          * Don't bother with the FDT, we no longer need it after this.
>          */
>         memset(init_idmap_pg_dir, 0,
> -              (u64)init_idmap_pg_dir - (u64)init_idmap_pg_end);
> +              (u64)init_idmap_pg_end - (u64)init_idmap_pg_dir);
>
>         create_init_idmap(init_idmap_pg_dir, mask);
>         dsb(ishst);
> --
> 2.33.0
>

Re: [PATCH] arm64: Clear the initial ID map correctly before remapping

[Zenghui Yu]

On 2024/6/21 17:52, Ard Biesheuvel wrote:
> On Fri, 21 Jun 2024 at 11:28, Zenghui Yu <yuzenghui@huawei.com> wrote:
> >
> > In the attempt to clear and recreate the initial ID map for LPA2, we
> > wrongly use 'start - end' as the map size and make the memset() almost a
> > nop.
> >
> > Fix it by passing the correct map size.
> >
> > Fixes: 9684ec186f8f ("arm64: Enable LPA2 at boot if supported by the system")
> > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> > ---
> >
> > Found by code inspection (don't have the appropriate HW to test it).
> >
> 
> Good catch!
> 
> Even though memset() takes an unsigned size_t, the zeroing path in
> arm64's memset.S does a signed compare on the provided size, and will
> zero at most 63 bytes if the size has the sign bit set. So in the end,
> it does not clear anything.

Yup! It took me a while to figure out why memset() with a VERY large
size doesn't corrupt the memory. ;-)

> Note that in this particular case, that
> doesn't actually matter - the memory is reused immediately to create
> another copy of the ID map, and any unused regions containing garbage
> will just be ignored.

Agreed. I can fold it into the commit message if Will/Catalin ask for a
respin. And it looks like an alternate "fix" would be just removing the
memset().

> Nonetheless,
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Thanks!

Zenghui

Re: [PATCH] arm64: Clear the initial ID map correctly before remapping

[Will Deacon]

On Fri, 21 Jun 2024 17:28:09 +0800, Zenghui Yu wrote:
> In the attempt to clear and recreate the initial ID map for LPA2, we
> wrongly use 'start - end' as the map size and make the memset() almost a
> nop.
> 
> Fix it by passing the correct map size.
> 
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Clear the initial ID map correctly before remapping
      https://git.kernel.org/arm64/c/ecc54006f158

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev