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/Ib02d004d435b2ed87603b858ef7480f7b1... --- 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();
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/Ib02d004d435b2ed87603b858ef7480f7b1...
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
Hi Peter,
Regardless of the nitpick below, the patch looks correct to me:
Reviewed-by: Alexandru Elisei alexandru.elisei@arm.com
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/Ib02d004d435b2ed87603b858ef7480f7b1...
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)) {
Both braces could have been kept (they define multiline blocks), and as a bonus the diff becomes only one line removed. Not sure that's worth a respin.
Thanks, Alex
- 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
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,
linux-stable-mirror@lists.linaro.org
-
Alexandru Elisei -
Catalin Marinas -
Peter Collingbourne -
Will Deacon