diff mbox series

[-next,3/4] arm64: mm: add support for page table check

Message ID 20220317141203.3646253-4-tongtiangen@huawei.com (mailing list archive)
State New, archived
Headers show
Series mm: page_table_check: add support on arm64 and riscv | expand

Commit Message

Tong Tiangen March 17, 2022, 2:12 p.m. UTC 
From: Kefeng Wang <wangkefeng.wang@huawei.com>

As commit d283d422c6c4 ("x86: mm: add x86_64 support for page table
check"), add some necessary page table check hooks into routines that
modify user page tables.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm64/Kconfig               |  1 +
 arch/arm64/include/asm/pgtable.h | 63 +++++++++++++++++++++++++++++---
 2 files changed, 59 insertions(+), 5 deletions(-)

Comments

Catalin Marinas March 17, 2022, 7 p.m. UTC | #1 
On Thu, Mar 17, 2022 at 02:12:02PM +0000, Tong Tiangen wrote:
> @@ -628,6 +647,25 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>  #define pud_leaf(pud)		pud_sect(pud)
>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
>  
> +#ifdef CONFIG_PAGE_TABLE_CHECK
> +static inline bool pte_user_accessible_page(pte_t pte)
> +{
> +	return (pte_val(pte) & PTE_VALID) && (pte_val(pte) & PTE_USER);
> +}

There is another class of user mappings, execute-only, that have both
PTE_USER and PTE_UXN cleared. So this logic should be:

	pte_valid(pte) && (pte_user(pte) || pte_user_exec(pte))

with pte_user() as:

#define pte_user(pte)	(!!(pte_val(pte) & PTE_USER))

Do we care about PROT_NONE mappings here? They have the valid bit
cleared but pte_present() is true.

> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> +	return pmd_leaf(pmd) && (pmd_val(pmd) & PTE_VALID) &&
> +		(pmd_val(pmd) & PTE_USER);
> +}

pmd_leaf() implies valid, so you can skip it if that's the aim.

Similar comment to the pte variant on execute-only and PROT_NONE
mappings.
Tong Tiangen March 18, 2022, 3:58 a.m. UTC | #2 
在 2022/3/18 3:00, Catalin Marinas 写道:
> On Thu, Mar 17, 2022 at 02:12:02PM +0000, Tong Tiangen wrote:
>> @@ -628,6 +647,25 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>   #define pud_leaf(pud)		pud_sect(pud)
>>   #define pud_valid(pud)		pte_valid(pud_pte(pud))
>>   
>> +#ifdef CONFIG_PAGE_TABLE_CHECK
>> +static inline bool pte_user_accessible_page(pte_t pte)
>> +{
>> +	return (pte_val(pte) & PTE_VALID) && (pte_val(pte) & PTE_USER);
>> +}
> 
> There is another class of user mappings, execute-only, that have both
> PTE_USER and PTE_UXN cleared. So this logic should be:
> 
> 	pte_valid(pte) && (pte_user(pte) || pte_user_exec(pte))
> 
> with pte_user() as:
> 
> #define pte_user(pte)	(!!(pte_val(pte) & PTE_USER))

Good suggestion, the PTC(page table check) can cover UXN page and 
pte_user(pte) helper is required.

> 
> Do we care about PROT_NONE mappings here? They have the valid bit
> cleared but pte_present() is true.
>

PTC will not check this special type(PROT_NONE) of page.

>> +static inline bool pmd_user_accessible_page(pmd_t pmd)
>> +{
>> +	return pmd_leaf(pmd) && (pmd_val(pmd) & PTE_VALID) &&
>> +		(pmd_val(pmd) & PTE_USER);
>> +}
> 
> pmd_leaf() implies valid, so you can skip it if that's the aim.

PTC only checks whether the memory block corresponding to the pmd_leaf 
type can access, for !pmd_leaf, PTC checks at the pte level. So i think 
this is necessary.

> 
> Similar comment to the pte variant on execute-only and PROT_NONE
> mappings

Same considerations as above.

Thanks.
Tong
>
Catalin Marinas March 18, 2022, 5:18 p.m. UTC | #3 
On Fri, Mar 18, 2022 at 11:58:22AM +0800, Tong Tiangen wrote:
> 在 2022/3/18 3:00, Catalin Marinas 写道:
> > On Thu, Mar 17, 2022 at 02:12:02PM +0000, Tong Tiangen wrote:
> > > @@ -628,6 +647,25 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
> > >   #define pud_leaf(pud)		pud_sect(pud)
> > >   #define pud_valid(pud)		pte_valid(pud_pte(pud))
> > > +#ifdef CONFIG_PAGE_TABLE_CHECK
> > > +static inline bool pte_user_accessible_page(pte_t pte)
> > > +{
> > > +	return (pte_val(pte) & PTE_VALID) && (pte_val(pte) & PTE_USER);
> > > +}
[...]
> > Do we care about PROT_NONE mappings here? They have the valid bit
> > cleared but pte_present() is true.
> > 
> 
> PTC will not check this special type(PROT_NONE) of page.

PROT_NONE is just a permission but since we don't have independent read
and write bits in the pte, we implement it as an invalid pte (bit 0
cleared). The other content of the pte is fine, so pte_pfn() should
still work. PTC could as well check this, I don't think it hurts.

> > > +static inline bool pmd_user_accessible_page(pmd_t pmd)
> > > +{
> > > +	return pmd_leaf(pmd) && (pmd_val(pmd) & PTE_VALID) &&
> > > +		(pmd_val(pmd) & PTE_USER);
> > > +}
> > 
> > pmd_leaf() implies valid, so you can skip it if that's the aim.
> 
> PTC only checks whether the memory block corresponding to the pmd_leaf type
> can access, for !pmd_leaf, PTC checks at the pte level. So i think this is
> necessary.

My point is that the (pmd_val(pmd) & PTE_VALID) check is superfluous
since that's covered by pmd_leaf() already.
Tong Tiangen March 21, 2022, 6:15 a.m. UTC | #4 
在 2022/3/19 1:18, Catalin Marinas 写道:
> On Fri, Mar 18, 2022 at 11:58:22AM +0800, Tong Tiangen wrote:
>> 在 2022/3/18 3:00, Catalin Marinas 写道:
>>> On Thu, Mar 17, 2022 at 02:12:02PM +0000, Tong Tiangen wrote:
>>>> @@ -628,6 +647,25 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>>>    #define pud_leaf(pud)		pud_sect(pud)
>>>>    #define pud_valid(pud)		pte_valid(pud_pte(pud))
>>>> +#ifdef CONFIG_PAGE_TABLE_CHECK
>>>> +static inline bool pte_user_accessible_page(pte_t pte)
>>>> +{
>>>> +	return (pte_val(pte) & PTE_VALID) && (pte_val(pte) & PTE_USER);
>>>> +}
> [...]
>>> Do we care about PROT_NONE mappings here? They have the valid bit
>>> cleared but pte_present() is true.
>>>
>>
>> PTC will not check this special type(PROT_NONE) of page.
> 
> PROT_NONE is just a permission but since we don't have independent read
> and write bits in the pte, we implement it as an invalid pte (bit 0
> cleared). The other content of the pte is fine, so pte_pfn() should
> still work. PTC could as well check this, I don't think it hurts.

You have a point and the logic should be:

	pte_present(pte) && (pte_user(pte) || pte_user_exec(pte))

> 
>>>> +static inline bool pmd_user_accessible_page(pmd_t pmd)
>>>> +{
>>>> +	return pmd_leaf(pmd) && (pmd_val(pmd) & PTE_VALID) &&
>>>> +		(pmd_val(pmd) & PTE_USER);
>>>> +}
>>>
>>> pmd_leaf() implies valid, so you can skip it if that's the aim.
>>
>> PTC only checks whether the memory block corresponding to the pmd_leaf type
>> can access, for !pmd_leaf, PTC checks at the pte level. So i think this is
>> necessary.
> 
> My point is that the (pmd_val(pmd) & PTE_VALID) check is superfluous
> since that's covered by pmd_leaf() already.

Oh，i got it，you're right and these will be fixed in v2.

Considering all your suggestions, The final logic should be:

+#define pte_user(pte)          (!!(pte_val(pte) & PTE_USER))

+#define pmd_user(pmd)		pte_user(pmd_pte(pmd))
+#define pmd_user_exec(pmd)	pte_user_exec(pmd_pte(pmd))

+#define pud_user(pud)          pte_user(pud_pte(pud))

+static inline bool pte_user_accessible_page(pte_t pte)
+{
+	return pte_present(pte) && (pte_user(pte)|| pte_user_exec(pte));
+}

+static inline bool pmd_user_accessible_page(pmd_t pmd)
+{
+	return pmd_present(pmd) && (pmd_user(pmd)|| pmd_user_exec(pmd));
+}

+static inline bool pud_user_accessible_page(pud_t pud)
+{
+	return pud_present(pud) && pud_user(pud);
+}

>
Catalin Marinas March 21, 2022, 4:40 p.m. UTC | #5 
On Mon, Mar 21, 2022 at 02:15:36PM +0800, Tong Tiangen wrote:
> Considering all your suggestions, The final logic should be:
> 
> +#define pte_user(pte)          (!!(pte_val(pte) & PTE_USER))
> 
> +#define pmd_user(pmd)		pte_user(pmd_pte(pmd))
> +#define pmd_user_exec(pmd)	pte_user_exec(pmd_pte(pmd))
> 
> +#define pud_user(pud)          pte_user(pud_pte(pud))
> 
> +static inline bool pte_user_accessible_page(pte_t pte)
> +{
> +	return pte_present(pte) && (pte_user(pte)|| pte_user_exec(pte));
> +}

This is fine.

> +static inline bool pmd_user_accessible_page(pmd_t pmd)
> +{
> +	return pmd_present(pmd) && (pmd_user(pmd)|| pmd_user_exec(pmd));
> +}

That's fine as well assuming that the function is only called on the
set_pmd_at() path where we know that the pmd would be a block mapping
(huge page). I think that's the case from a quick look at the current
x86 implementation.

> +static inline bool pud_user_accessible_page(pud_t pud)
> +{
> +	return pud_present(pud) && pud_user(pud);
> +}

Same here.
Tong Tiangen March 22, 2022, 3:07 a.m. UTC | #6 
在 2022/3/22 0:40, Catalin Marinas 写道:
> On Mon, Mar 21, 2022 at 02:15:36PM +0800, Tong Tiangen wrote:
>> Considering all your suggestions, The final logic should be:
>>
>> +#define pte_user(pte)          (!!(pte_val(pte) & PTE_USER))
>>
>> +#define pmd_user(pmd)		pte_user(pmd_pte(pmd))
>> +#define pmd_user_exec(pmd)	pte_user_exec(pmd_pte(pmd))
>>
>> +#define pud_user(pud)          pte_user(pud_pte(pud))
>>
>> +static inline bool pte_user_accessible_page(pte_t pte)
>> +{
>> +	return pte_present(pte) && (pte_user(pte)|| pte_user_exec(pte));
>> +}
> 
> This is fine.
> 
>> +static inline bool pmd_user_accessible_page(pmd_t pmd)
>> +{
>> +	return pmd_present(pmd) && (pmd_user(pmd)|| pmd_user_exec(pmd));
>> +}
> 
> That's fine as well assuming that the function is only called on the
> set_pmd_at() path where we know that the pmd would be a block mapping
> (huge page). I think that's the case from a quick look at the current
> x86 implementation.

Yeah, PTC only check pmd block mapping(huge page) and pud is similar.
These code logic will be included in V2.

Thanks.

> 
>> +static inline bool pud_user_accessible_page(pud_t pud)
>> +{
>> +	return pud_present(pud) && pud_user(pud);
>> +}
> 
> Same here.
>
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 962c84952c98..1880dd0a8a11 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -91,6 +91,7 @@  config ARM64
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
 	select ARCH_SUPPORTS_NUMA_BALANCING
+	select ARCH_SUPPORTS_PAGE_TABLE_CHECK
 	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION if COMPAT
 	select ARCH_WANT_DEFAULT_BPF_JIT
 	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 94e147e5456c..2ea6b5e268d0 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -33,6 +33,7 @@ 
 #include <linux/mmdebug.h>
 #include <linux/mm_types.h>
 #include <linux/sched.h>
+#include <linux/page_table_check.h>
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE
@@ -312,7 +313,7 @@  static inline void __check_racy_pte_update(struct mm_struct *mm, pte_t *ptep,
 		     __func__, pte_val(old_pte), pte_val(pte));
 }
 
-static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
+static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
 			      pte_t *ptep, pte_t pte)
 {
 	if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte))
@@ -343,6 +344,13 @@  static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
 	set_pte(ptep, pte);
 }
 
+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
+			      pte_t *ptep, pte_t pte)
+{
+	page_table_check_pte_set(mm, addr, ptep, pte);
+	return __set_pte_at(mm, addr, ptep, pte);
+}
+
 /*
  * Huge pte definitions.
  */
@@ -485,8 +493,19 @@  static inline pmd_t pmd_mkdevmap(pmd_t pmd)
 #define pud_pfn(pud)		((__pud_to_phys(pud) & PUD_MASK) >> PAGE_SHIFT)
 #define pfn_pud(pfn,prot)	__pud(__phys_to_pud_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
 
-#define set_pmd_at(mm, addr, pmdp, pmd)	set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd))
-#define set_pud_at(mm, addr, pudp, pud)	set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud))
+static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
+			      pmd_t *pmdp, pmd_t pmd)
+{
+	page_table_check_pmd_set(mm, addr, pmdp, pmd);
+	return __set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd));
+}
+
+static inline void set_pud_at(struct mm_struct *mm, unsigned long addr,
+			      pud_t *pudp, pud_t pud)
+{
+	page_table_check_pud_set(mm, addr, pudp, pud);
+	return __set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud));
+}
 
 #define __p4d_to_phys(p4d)	__pte_to_phys(p4d_pte(p4d))
 #define __phys_to_p4d_val(phys)	__phys_to_pte_val(phys)
@@ -628,6 +647,25 @@  static inline unsigned long pmd_page_vaddr(pmd_t pmd)
 #define pud_leaf(pud)		pud_sect(pud)
 #define pud_valid(pud)		pte_valid(pud_pte(pud))
 
+#ifdef CONFIG_PAGE_TABLE_CHECK
+static inline bool pte_user_accessible_page(pte_t pte)
+{
+	return (pte_val(pte) & PTE_VALID) && (pte_val(pte) & PTE_USER);
+}
+
+static inline bool pmd_user_accessible_page(pmd_t pmd)
+{
+	return pmd_leaf(pmd) && (pmd_val(pmd) & PTE_VALID) &&
+		(pmd_val(pmd) & PTE_USER);
+}
+
+static inline bool pud_user_accessible_page(pud_t pud)
+{
+	return pud_leaf(pud) && (pud_val(pud) & PTE_VALID) &&
+		(pud_val(pud) & PTE_USER);
+}
+#endif
+
 static inline void set_pud(pud_t *pudp, pud_t pud)
 {
 #ifdef __PAGETABLE_PUD_FOLDED
@@ -856,11 +894,21 @@  static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
+static inline pte_t __ptep_get_and_clear(struct mm_struct *mm,
+				       unsigned long address, pte_t *ptep)
+{
+	return __pte(xchg_relaxed(&pte_val(*ptep), 0));
+}
+
 #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
 static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
 				       unsigned long address, pte_t *ptep)
 {
-	return __pte(xchg_relaxed(&pte_val(*ptep), 0));
+	pte_t pte = __ptep_get_and_clear(mm, address, ptep);
+
+	page_table_check_pte_clear(mm, address, pte);
+
+	return pte;
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
@@ -868,7 +916,11 @@  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
 static inline pmd_t pmdp_huge_get_and_clear(struct mm_struct *mm,
 					    unsigned long address, pmd_t *pmdp)
 {
-	return pte_pmd(ptep_get_and_clear(mm, address, (pte_t *)pmdp));
+	pmd_t pmd = pte_pmd(__ptep_get_and_clear(mm, address, (pte_t *)pmdp));
+
+	page_table_check_pmd_clear(mm, address, pmd);
+
+	return pmd;
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
@@ -902,6 +954,7 @@  static inline void pmdp_set_wrprotect(struct mm_struct *mm,
 static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
 		unsigned long address, pmd_t *pmdp, pmd_t pmd)
 {
+	page_table_check_pmd_set(vma->vm_mm, address, pmdp, pmd);
 	return __pmd(xchg_relaxed(&pmd_val(*pmdp), pmd_val(pmd)));
 }
 #endif