[PATCH] arm64: mte: Do not set PG_mte_tagged if tags were not initialized

[PATCH] arm64: mte: Do not set PG_mte_tagged if tags were not initialized

[Peter Collingbourne]

The mte_sync_page_tags() function sets PG_mte_tagged if it initializes
page tags. Then we return to mte_sync_tags(), which sets PG_mte_tagged
again. At best, this is redundant. However, it is possible for
mte_sync_page_tags() to return without having initialized tags for the
page, i.e. in the case where check_swap is true (non-compound page),
is_swap_pte(old_pte) is false and pte_is_tagged is false. So at worst,
we set PG_mte_tagged on a page with uninitialized tags. This can happen
if, for example, page migration causes a PTE for an untagged page to
be replaced. If the userspace program subsequently uses mprotect() to
enable PROT_MTE for that page, the uninitialized tags will be exposed
to userspace.

Fix it by removing the redundant call to set_page_mte_tagged().

Fixes: e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
Signed-off-by: Peter Collingbourne <pcc@google.com>
Cc: <stable@vger.kernel.org> # 6.1
Link: https://linux-review.googlesource.com/id/Ib02d004d435b2ed87603b858ef7480f7b1463052
---
 arch/arm64/kernel/mte.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index f5bcb0dc6267..7e89968bd282 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -66,13 +66,10 @@ void mte_sync_tags(pte_t old_pte, pte_t pte)
 		return;
 
 	/* if PG_mte_tagged is set, tags have already been initialised */
-	for (i = 0; i < nr_pages; i++, page++) {
-		if (!page_mte_tagged(page)) {
+	for (i = 0; i < nr_pages; i++, page++)
+		if (!page_mte_tagged(page))
 			mte_sync_page_tags(page, old_pte, check_swap,
 					   pte_is_tagged);
-			set_page_mte_tagged(page);
-		}
-	}
 
 	/* ensure the tags are visible before the PTE is set */
 	smp_wmb();
-- 
2.40.0.634.g4ca3ef3211-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: mte: Do not set PG_mte_tagged if tags were not initialized

[Catalin Marinas]

On Thu, Apr 20, 2023 at 02:43:27PM -0700, Peter Collingbourne wrote:
> The mte_sync_page_tags() function sets PG_mte_tagged if it initializes
> page tags. Then we return to mte_sync_tags(), which sets PG_mte_tagged
> again. At best, this is redundant. However, it is possible for
> mte_sync_page_tags() to return without having initialized tags for the
> page, i.e. in the case where check_swap is true (non-compound page),
> is_swap_pte(old_pte) is false and pte_is_tagged is false. So at worst,
> we set PG_mte_tagged on a page with uninitialized tags. This can happen
> if, for example, page migration causes a PTE for an untagged page to
> be replaced. If the userspace program subsequently uses mprotect() to
> enable PROT_MTE for that page, the uninitialized tags will be exposed
> to userspace.
> 
> Fix it by removing the redundant call to set_page_mte_tagged().
> 
> Fixes: e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Cc: <stable@vger.kernel.org> # 6.1
> Link: https://linux-review.googlesource.com/id/Ib02d004d435b2ed87603b858ef7480f7b1463052
> ---
>  arch/arm64/kernel/mte.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index f5bcb0dc6267..7e89968bd282 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -66,13 +66,10 @@ void mte_sync_tags(pte_t old_pte, pte_t pte)
>  		return;
>  
>  	/* if PG_mte_tagged is set, tags have already been initialised */
> -	for (i = 0; i < nr_pages; i++, page++) {
> -		if (!page_mte_tagged(page)) {
> +	for (i = 0; i < nr_pages; i++, page++)
> +		if (!page_mte_tagged(page))
>  			mte_sync_page_tags(page, old_pte, check_swap,
>  					   pte_is_tagged);
> -			set_page_mte_tagged(page);
> -		}
> -	}

It makes sense, not sure why I added it here when mte_sync_page_tags()
was already setting the flag if needed.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: mte: Do not set PG_mte_tagged if tags were not initialized

[Will Deacon]

On Thu, 20 Apr 2023 14:43:27 -0700, Peter Collingbourne wrote:
> The mte_sync_page_tags() function sets PG_mte_tagged if it initializes
> page tags. Then we return to mte_sync_tags(), which sets PG_mte_tagged
> again. At best, this is redundant. However, it is possible for
> mte_sync_page_tags() to return without having initialized tags for the
> page, i.e. in the case where check_swap is true (non-compound page),
> is_swap_pte(old_pte) is false and pte_is_tagged is false. So at worst,
> we set PG_mte_tagged on a page with uninitialized tags. This can happen
> if, for example, page migration causes a PTE for an untagged page to
> be replaced. If the userspace program subsequently uses mprotect() to
> enable PROT_MTE for that page, the uninitialized tags will be exposed
> to userspace.
> 
> [...]

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

[1/1] arm64: mte: Do not set PG_mte_tagged if tags were not initialized
      https://git.kernel.org/arm64/c/c4c597f1b367

Cheers,
-- 
Will

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel