mm, pagemap: Hide swap entry for unprivileged users

PATCH In review Huang, Ying 4 participants 4 messages May 8, 2018 – May 9, 2018

Subsystems (3)

Memory management Proc filesystem Memory mapping

lore

Share & tools

lore

Huang, Ying

author

2018-05-08 01:28 UTC #0 lore raw

PATCH 1/1 mm, pagemap: Hide swap entry for unprivileged users

From: Huang Ying <[email protected]>

In ab676b7d6fbf ("pagemap: do not leak physical addresses to non-privileged userspace"), the /proc/PID/pagemap is restricted to be readable only by CAP_SYS_ADMIN to address some security issue. In 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users"), the restriction is relieved to make /proc/PID/pagemap readable, but hide the physical addresses for non-privileged users. But the swap entries are readable for non-privileged users too. This has some security issues. For example, for page under migrating, the swap entry has physical address information. So, in this patch, the swap entries are hided for non-privileged users too.

Fixes 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users")

Signed-off-by "Huang, Ying" <[email protected]>

Cc Konstantin Khlebnikov <[email protected]>

Cc Andrei Vagin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Jerome Glisse <[email protected]>

Cc Daniel Colascione <[email protected]>

Cc Zi Yan <[email protected]>

Cc Naoya Horiguchi <[email protected]>

---
 fs/proc/task_mmu.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

fs/proc/task_mmu.c +16 ~~-10~~

index a20c6e495bb2..ff947fdd7c71 100644
@@ -1258,8 +1258,9 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
 		if (pte_swp_soft_dirty(pte))
 			flags |= PM_SOFT_DIRTY;
 		entry = pte_to_swp_entry(pte);
-		frame = swp_type(entry) |
-			(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
+		if (pm->show_pfn)
+			frame = swp_type(entry) |
+				(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
 		flags |= PM_SWAP;
 		if (is_migration_entry(entry))
 			page = migration_entry_to_page(entry);

@@ -1310,11 +1311,14 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
 		else if (is_swap_pmd(pmd)) {
 			swp_entry_t entry = pmd_to_swp_entry(pmd);
-			unsigned long offset = swp_offset(entry);
+			unsigned long offset;
 
-			offset += (addr & ~PMD_MASK) >> PAGE_SHIFT;
-			frame = swp_type(entry) |
-				(offset << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				offset = swp_offset(entry) +
+					((addr & ~PMD_MASK) >> PAGE_SHIFT);
+				frame = swp_type(entry) |
+					(offset << MAX_SWAPFILES_SHIFT);
+			}
 			flags |= PM_SWAP;
 			if (pmd_swp_soft_dirty(pmd))
 				flags |= PM_SOFT_DIRTY;

@@ -1332,10 +1336,12 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 			err = add_to_pagemap(addr, &pme, pm);
 			if (err)
 				break;
-			if (pm->show_pfn && (flags & PM_PRESENT))
-				frame++;
-			else if (flags & PM_SWAP)
-				frame += (1 << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				if (flags & PM_PRESENT)
+					frame++;
+				else if (flags & PM_SWAP)
+					frame += (1 << MAX_SWAPFILES_SHIFT);
+			}
 		}
 		spin_unlock(ptl);
 		return err;

Signature · 1 line

2.17.0

Naoya Horiguchi

Reviewed-by on v1 1/1

2018-05-08 09:09 UTC #1 lore raw

Quoted 13 lines · Ying

From: Huang Ying <[email protected]>
In ab676b7d6fbf ("pagemap: do not leak physical addresses to non-privileged userspace"), the /proc/PID/pagemap is restricted to be readable only by CAP_SYS_ADMIN to address some security issue. In 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users"), the restriction is relieved to make /proc/PID/pagemap readable, but hide the physical addresses for non-privileged users. But the swap entries are readable for non-privileged users too. This has some security issues. For example, for page under migrating, the swap entry has physical address information. So, in this patch, the swap entries are hided for non-privileged users too.

Quoted 10 lines

Fixes 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users")
Signed-off-by "Huang, Ying" <[email protected]>
Suggested-by Kirill A. Shutemov <[email protected]>
Cc Konstantin Khlebnikov <[email protected]>
Cc Andrei Vagin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Jerome Glisse <[email protected]>
Cc Daniel Colascione <[email protected]>
Cc Zi Yan <[email protected]>
Cc Naoya Horiguchi <[email protected]>

Hi ying huang,

This patch looks good to me.

Reviewed-by Naoya Horiguchi <[email protected]>

Quoted 60 lines

---
 fs/proc/task_mmu.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

fs/proc/task_mmu.c +16 ~~-10~~

index a20c6e495bb2..ff947fdd7c71 100644
@@ -1258,8 +1258,9 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
 		if (pte_swp_soft_dirty(pte))
 			flags |= PM_SOFT_DIRTY;
 		entry = pte_to_swp_entry(pte);
-		frame = swp_type(entry) |
-			(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
+		if (pm->show_pfn)
+			frame = swp_type(entry) |
+				(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
 		flags |= PM_SWAP;
 		if (is_migration_entry(entry))
 			page = migration_entry_to_page(entry);

@@ -1310,11 +1311,14 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
 		else if (is_swap_pmd(pmd)) {
 			swp_entry_t entry = pmd_to_swp_entry(pmd);
-			unsigned long offset = swp_offset(entry);
+			unsigned long offset;
 
-			offset += (addr & ~PMD_MASK) >> PAGE_SHIFT;
-			frame = swp_type(entry) |
-				(offset << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				offset = swp_offset(entry) +
+					((addr & ~PMD_MASK) >> PAGE_SHIFT);
+				frame = swp_type(entry) |
+					(offset << MAX_SWAPFILES_SHIFT);
+			}
 			flags |= PM_SWAP;
 			if (pmd_swp_soft_dirty(pmd))
 				flags |= PM_SOFT_DIRTY;

@@ -1332,10 +1336,12 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 			err = add_to_pagemap(addr, &pme, pm);
 			if (err)
 				break;
-			if (pm->show_pfn && (flags & PM_PRESENT))
-				frame++;
-			else if (flags & PM_SWAP)
-				frame += (1 << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				if (flags & PM_PRESENT)
+					frame++;
+				else if (flags & PM_SWAP)
+					frame += (1 << MAX_SWAPFILES_SHIFT);
+			}
 		}
 		spin_unlock(ptl);
 		return err;

Signature · 1 line

2.17.0

Konstantin Khlebnikov

Reviewed-by on v1 1/1

2018-05-08 11:17 UTC #2 lore raw

In reply to Huang, Ying

Quoted 13 lines · Ying

From: Huang Ying <[email protected]>
In ab676b7d6fbf ("pagemap: do not leak physical addresses to non-privileged userspace"), the /proc/PID/pagemap is restricted to be readable only by CAP_SYS_ADMIN to address some security issue. In 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users"), the restriction is relieved to make /proc/PID/pagemap readable, but hide the physical addresses for non-privileged users. But the swap entries are readable for non-privileged users too. This has some security issues. For example, for page under migrating, the swap entry has physical address information. So, in this patch, the swap entries are hided for non-privileged users too.

Quoted 10 lines

Fixes 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users")
Signed-off-by "Huang, Ying" <[email protected]>
Suggested-by Kirill A. Shutemov <[email protected]>
Cc Konstantin Khlebnikov <[email protected]>
Cc Andrei Vagin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Jerome Glisse <[email protected]>
Cc Daniel Colascione <[email protected]>
Cc Zi Yan <[email protected]>
Cc Naoya Horiguchi <[email protected]>

Looks good.

Reviewed-by Konstantin Khlebnikov <[email protected]>

Quoted 57 lines

---
  fs/proc/task_mmu.c | 26 ++++++++++++++++----------
  1 file changed, 16 insertions(+), 10 deletions(-)

fs/proc/task_mmu.c +16 ~~-10~~

index a20c6e495bb2..ff947fdd7c71 100644
@@ -1258,8 +1258,9 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
  		if (pte_swp_soft_dirty(pte))
  			flags |= PM_SOFT_DIRTY;
  		entry = pte_to_swp_entry(pte);
-		frame = swp_type(entry) |
-			(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
+		if (pm->show_pfn)
+			frame = swp_type(entry) |
+				(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
  		flags |= PM_SWAP;
  		if (is_migration_entry(entry))
  			page = migration_entry_to_page(entry);

@@ -1310,11 +1311,14 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
  #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
  		else if (is_swap_pmd(pmd)) {
  			swp_entry_t entry = pmd_to_swp_entry(pmd);
-			unsigned long offset = swp_offset(entry);
+			unsigned long offset;
 
-			offset += (addr & ~PMD_MASK) >> PAGE_SHIFT;
-			frame = swp_type(entry) |
-				(offset << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				offset = swp_offset(entry) +
+					((addr & ~PMD_MASK) >> PAGE_SHIFT);
+				frame = swp_type(entry) |
+					(offset << MAX_SWAPFILES_SHIFT);
+			}
  			flags |= PM_SWAP;
  			if (pmd_swp_soft_dirty(pmd))
  				flags |= PM_SOFT_DIRTY;

@@ -1332,10 +1336,12 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
  			err = add_to_pagemap(addr, &pme, pm);
  			if (err)
  				break;
-			if (pm->show_pfn && (flags & PM_PRESENT))
-				frame++;
-			else if (flags & PM_SWAP)
-				frame += (1 << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				if (flags & PM_PRESENT)
+					frame++;
+				else if (flags & PM_SWAP)
+					frame += (1 << MAX_SWAPFILES_SHIFT);
+			}
  		}
  		spin_unlock(ptl);
  		return err;

Michal Hocko

Acked-by on v1 1/1

2018-05-09 08:16 UTC #3 lore raw

In reply to Huang, Ying

Quoted 12 lines · Ying

From: Huang Ying <[email protected]>
In ab676b7d6fbf ("pagemap: do not leak physical addresses to non-privileged userspace"), the /proc/PID/pagemap is restricted to be readable only by CAP_SYS_ADMIN to address some security issue. In 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users"), the restriction is relieved to make /proc/PID/pagemap readable, but hide the physical addresses for non-privileged users. But the swap entries are readable for non-privileged users too. This has some security issues. For example, for page under migrating, the swap entry has physical address information. So, in this patch, the swap entries are hided for non-privileged users too.

Migration entries are quite ephemeral so I am not sure how this could be abused. But I do agree that hiding swap entries make some sense from consistency POV

Quoted 10 lines

Fixes 1c90308e7a77 ("pagemap: hide physical addresses from non-privileged users")
Signed-off-by "Huang, Ying" <[email protected]>
Suggested-by Kirill A. Shutemov <[email protected]>
Cc Konstantin Khlebnikov <[email protected]>
Cc Andrei Vagin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Jerome Glisse <[email protected]>
Cc Daniel Colascione <[email protected]>
Cc Zi Yan <[email protected]>
Cc Naoya Horiguchi <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 59 lines

---
 fs/proc/task_mmu.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

fs/proc/task_mmu.c +16 ~~-10~~

index a20c6e495bb2..ff947fdd7c71 100644
@@ -1258,8 +1258,9 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
 		if (pte_swp_soft_dirty(pte))
 			flags |= PM_SOFT_DIRTY;
 		entry = pte_to_swp_entry(pte);
-		frame = swp_type(entry) |
-			(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
+		if (pm->show_pfn)
+			frame = swp_type(entry) |
+				(swp_offset(entry) << MAX_SWAPFILES_SHIFT);
 		flags |= PM_SWAP;
 		if (is_migration_entry(entry))
 			page = migration_entry_to_page(entry);

@@ -1310,11 +1311,14 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
 		else if (is_swap_pmd(pmd)) {
 			swp_entry_t entry = pmd_to_swp_entry(pmd);
-			unsigned long offset = swp_offset(entry);
+			unsigned long offset;
 
-			offset += (addr & ~PMD_MASK) >> PAGE_SHIFT;
-			frame = swp_type(entry) |
-				(offset << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				offset = swp_offset(entry) +
+					((addr & ~PMD_MASK) >> PAGE_SHIFT);
+				frame = swp_type(entry) |
+					(offset << MAX_SWAPFILES_SHIFT);
+			}
 			flags |= PM_SWAP;
 			if (pmd_swp_soft_dirty(pmd))
 				flags |= PM_SOFT_DIRTY;

@@ -1332,10 +1336,12 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
 			err = add_to_pagemap(addr, &pme, pm);
 			if (err)
 				break;
-			if (pm->show_pfn && (flags & PM_PRESENT))
-				frame++;
-			else if (flags & PM_SWAP)
-				frame += (1 << MAX_SWAPFILES_SHIFT);
+			if (pm->show_pfn) {
+				if (flags & PM_PRESENT)
+					frame++;
+				else if (flags & PM_SWAP)
+					frame += (1 << MAX_SWAPFILES_SHIFT);
+			}
 		}
 		spin_unlock(ptl);
 		return err;

Signature · 1 line

2.17.0

Signature · 2 lines

Michal Hocko
SUSE Labs