arm64: mm: assign PXN permission in trans_pgd

trans_pgd_create_copy() can hit "VM_BUG_ON(mm != &init_mm)" in the function pmd_populate_kernel().

p?d_populate() helpers resort to the input parameter mm_struct to decide the attribute: UXN or PXN. And plus the fact, either hibernation or kexec runs in privilege mode. So the bug can be fixed by assigning &init_mm to the callsites of these helpers.

Signed-off-by Pingfan Liu <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Pasha Tatashin <[email protected]>

Cc James Morse <[email protected]>

Cc Matthias Brugger <[email protected]>

To: [email protected]

---
 arch/arm64/mm/trans_pgd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/arm64/mm/trans_pgd.c +0 -0

index d7da8ca40d2e..7b43d179e94d 100644

Quoted 2 lines · Pingfan Liu

trans_pgd_create_copy() can hit "VM_BUG_ON(mm != &init_mm)" in the function pmd_populate_kernel().

I think that's more important to fix than the PXN. You may want to change the subject to something like "Fix VM_BUG_ON ..."

Quoted 4 lines

p?d_populate() helpers resort to the input parameter mm_struct to decide the attribute: UXN or PXN. And plus the fact, either hibernation or kexec runs in privilege mode. So the bug can be fixed by assigning &init_mm to the callsites of these helpers.

That's the table PXN, so it's not a big deal if we miss it.

Quoted 7 lines

Signed-off-by Pingfan Liu <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Pasha Tatashin <[email protected]>

Cc James Morse <[email protected]>

Cc Matthias Brugger <[email protected]>

To: [email protected]

A Fixes: tag would be nice, we just need to decide which.

Quoted 9 lines

---
 arch/arm64/mm/trans_pgd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/arm64/mm/trans_pgd.c +0 -0

index d7da8ca40d2e..7b43d179e94d 100644

arch/arm64/mm/trans_pgd.c static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp, · line 71

Quoted 6 lines

arch/arm64/mm/trans_pgd.c +1 -1

@@ -67,7 +67,7 @@ static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp,
      dst_ptep = trans_alloc(info);
      if (!dst_ptep)
              return -ENOMEM;
-     pmd_populate_kernel(NULL, dst_pmdp, dst_ptep);
+     pmd_populate_kernel(&init_mm, dst_pmdp, dst_ptep);
      dst_ptep = pte_offset_kernel(dst_pmdp, start);

Commit 5de59884ac0e ("arm64: trans_pgd: pass NULL instead of init_mm to *_populate functions") actually replaced &init_mm with NULL. Commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings") introduced the VM_BUG_ON.

So we either revert Pasha's trans_pgd commit (as per your patch) or we fix Ard's commit to make it aware of a NULL mm and either skip the table UXN altogether or treat it as init_mm.

Quoted 18 lines · Catalin Marinas

arch/arm64/mm/trans_pgd.c static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp, · line 71

Quoted 11 lines · Pingfan Liu

arch/arm64/mm/trans_pgd.c +1 -1

index d7da8ca40d2e..7b43d179e94d 100644
@@ -67,7 +67,7 @@ static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp,
      dst_ptep = trans_alloc(info);
      if (!dst_ptep)
              return -ENOMEM;
-     pmd_populate_kernel(NULL, dst_pmdp, dst_ptep);
+     pmd_populate_kernel(&init_mm, dst_pmdp, dst_ptep);
      dst_ptep = pte_offset_kernel(dst_pmdp, start);

Commit 5de59884ac0e ("arm64: trans_pgd: pass NULL instead of init_mm to *_populate functions") actually replaced &init_mm with NULL. Commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings") introduced the VM_BUG_ON.

Quoted 4 lines

So we either revert Pasha's trans_pgd commit (as per your patch) or we fix Ard's commit to make it aware of a NULL mm and either skip the table UXN altogether or treat it as init_mm.

That code in pmd_populate_kernel() does not actually *use* the mm argument at all, it just BUGs if its value != init_mm. So we can just relax the BUG() to disregard mm == NULL.

BTW, in line with PAGE_KERNEL, pud_populate() could set both UXN and PXN for init_mm mappings.

Quoted 18 lines · Ard Biesheuvel

Quoted 17 lines · Catalin Marinas

arch/arm64/mm/trans_pgd.c static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp, · line 71

Quoted 11 lines · Pingfan Liu

arch/arm64/mm/trans_pgd.c +1 -1

index d7da8ca40d2e..7b43d179e94d 100644
@@ -67,7 +67,7 @@ static int copy_pte(struct trans_pgd_info *info, pmd_t *dst_pmdp,
      dst_ptep = trans_alloc(info);
      if (!dst_ptep)
              return -ENOMEM;
-     pmd_populate_kernel(NULL, dst_pmdp, dst_ptep);
+     pmd_populate_kernel(&init_mm, dst_pmdp, dst_ptep);
      dst_ptep = pte_offset_kernel(dst_pmdp, start);

Commit 5de59884ac0e ("arm64: trans_pgd: pass NULL instead of init_mm to *_populate functions") actually replaced &init_mm with NULL. Commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings") introduced the VM_BUG_ON.

Quoted 9 lines

Quoted 5 lines

So we either revert Pasha's trans_pgd commit (as per your patch) or we fix Ard's commit to make it aware of a NULL mm and either skip the table UXN altogether or treat it as init_mm.

That code in pmd_populate_kernel() does not actually *use* the mm argument at all, it just BUGs if its value != init_mm. So we can just relax the BUG() to disregard mm == NULL.

We also have pud_populate() that ends up setting PUD_TABLE_PXN when mm is NULL. Fortunately we don't execute anything from those mappings, they are just a copy of the linear map.

BTW, in line with PAGE_KERNEL, pud_populate() could set both UXN and PXN for init_mm mappings.

Signature · 6 lines

Catalin

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

trans_pgd_create_copy() can hit "VM_BUG_ON(mm != &init_mm)" in the function pmd_populate_kernel().

This is the combined consequence of commit 5de59884ac0e ("arm64:
trans_pgd: pass NULL instead of init_mm to *_populate functions"), which replaced &init_mm with NULL and commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings"), which introduced the VM_BUG_ON.

Since the former sounds reasonable, it is better to work on the later. From the perspective of trans_pgd, two groups of functions are considered in the later one:

pmd_populate_kernel()

    mm == NULL should be fixed, else it hits VM_BUG_ON()

p?d_populate()
mm == NULL means PXN, that is OK, since trans_pgd only copies a linear map, no execution will happen on the map.

So it is good enough to just relax VM_BUG_ON() to disregard mm == NULL

Fixes 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings")

Signed-off-by Pingfan Liu <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Will Deacon <[email protected]>

Cc Pasha Tatashin <[email protected]>

Cc James Morse <[email protected]>

Cc Matthias Brugger <[email protected]>

To: [email protected]

trans_pgd_create_copy() can hit "VM_BUG_ON(mm != &init_mm)" in the function pmd_populate_kernel().

This is the combined consequence of commit 5de59884ac0e ("arm64:
trans_pgd: pass NULL instead of init_mm to *_populate functions"), which replaced &init_mm with NULL and commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings"), which introduced the VM_BUG_ON.

Since the former sounds reasonable, it is better to work on the later. From the perspective of trans_pgd, two groups of functions are considered in the later one:

pmd_populate_kernel()

    mm == NULL should be fixed, else it hits VM_BUG_ON()

p?d_populate()
mm == NULL means PXN, that is OK, since trans_pgd only copies a linear map, no execution will happen on the map.

So it is good enough to just relax VM_BUG_ON() to disregard mm == NULL

Fixes 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings")

Signed-off-by Pingfan Liu <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Will Deacon <[email protected]>

Cc Pasha Tatashin <[email protected]>

Cc James Morse <[email protected]>

Cc Matthias Brugger <[email protected]>

To: [email protected]

---
 arch/arm64/include/asm/pgalloc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/include/asm/pgalloc.h +0 -0

index 8433a2058eb1..237224484d0f 100644

trans_pgd_create_copy() can hit "VM_BUG_ON(mm != &init_mm)" in the function pmd_populate_kernel().

This is the combined consequence of commit 5de59884ac0e ("arm64:
trans_pgd: pass NULL instead of init_mm to *_populate functions"), which replaced &init_mm with NULL and commit 59511cfd08f3 ("arm64: mm: use XN table mapping attributes for user/kernel mappings"), which introduced the VM_BUG_ON.

[...]