[PATCH 09/10] sparc64: Add basic validations to {pud,pmd}_bad().

[PATCH 09/10] sparc64: Add basic validations to {pud,pmd}_bad().

[David Miller]

Instead of returning false we should at least check the most basic
things, otherwise page table corruptions will be very difficult to
debug.

PMD and PTE tables are of size PAGE_SIZE, so none of the sub-PAGE_SIZE
bits should be set.

We also complement this with a check that the physical address the
pud/pmd points to is valid memory.

PowerPC was used as a guide while implementating this.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/pgtable_64.h | 46 +++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
index ff97960..fd4dbdd 100644
--- a/arch/sparc/include/asm/pgtable_64.h
+++ b/arch/sparc/include/asm/pgtable_64.h
@@ -71,6 +71,23 @@
 
 #include <linux/sched.h>
 
+extern unsigned long sparc64_valid_addr_bitmap[];
+
+/* Needs to be defined here and not in linux/mm.h, as it is arch dependent */
+static inline bool __kern_addr_valid(unsigned long paddr)
+{
+	if ((paddr >> MAX_PHYS_ADDRESS_BITS) != 0UL)
+		return false;
+	return test_bit(paddr >> 22, sparc64_valid_addr_bitmap);
+}
+
+static inline bool kern_addr_valid(unsigned long addr)
+{
+	unsigned long paddr = __pa(addr);
+
+	return __kern_addr_valid(paddr);
+}
+
 /* Entries per page directory level. */
 #define PTRS_PER_PTE	(1UL << (PAGE_SHIFT-3))
 #define PTRS_PER_PMD	(1UL << PMD_BITS)
@@ -743,6 +760,20 @@ static inline int pmd_present(pmd_t pmd)
 
 #define pmd_none(pmd)			(!pmd_val(pmd))
 
+/* pmd_bad() is only called on non-trans-huge PMDs.  Our encoding is
+ * very simple, it's just the physical address.  PTE tables are of
+ * size PAGE_SIZE so make sure the sub-PAGE_SIZE bits are clear and
+ * the top bits outside of the range of any physical address size we
+ * support are clear as well.  We also validate the physical itself.
+ */
+#define pmd_bad(pmd)			((pmd_val(pmd) & ~PAGE_MASK) || \
+					 !__kern_addr_valid(pmd_val(pmd)))
+
+#define pud_none(pud)			(!pud_val(pud))
+
+#define pud_bad(pud)			((pud_val(pud) & ~PAGE_MASK) || \
+					 !__kern_addr_valid(pud_val(pud)))
+
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 extern void set_pmd_at(struct mm_struct *mm, unsigned long addr,
 		       pmd_t *pmdp, pmd_t pmd);
@@ -776,10 +807,7 @@ static inline unsigned long __pmd_page(pmd_t pmd)
 #define pud_page_vaddr(pud)		\
 	((unsigned long) __va(pud_val(pud)))
 #define pud_page(pud) 			virt_to_page((void *)pud_page_vaddr(pud))
-#define pmd_bad(pmd)			(0)
 #define pmd_clear(pmdp)			(pmd_val(*(pmdp)) = 0UL)
-#define pud_none(pud)			(!pud_val(pud))
-#define pud_bad(pud)			(0)
 #define pud_present(pud)		(pud_val(pud) != 0U)
 #define pud_clear(pudp)			(pud_val(*(pudp)) = 0UL)
 
@@ -909,18 +937,6 @@ extern unsigned long pte_file(pte_t);
 extern pte_t pgoff_to_pte(unsigned long);
 #define PTE_FILE_MAX_BITS	(64UL - PAGE_SHIFT - 1UL)
 
-extern unsigned long sparc64_valid_addr_bitmap[];
-
-/* Needs to be defined here and not in linux/mm.h, as it is arch dependent */
-static inline bool kern_addr_valid(unsigned long addr)
-{
-	unsigned long paddr = __pa(addr);
-
-	if ((paddr >> MAX_PHYS_ADDRESS_BITS) != 0UL)
-		return false;
-	return test_bit(paddr >> 22, sparc64_valid_addr_bitmap);
-}
-
 extern int page_in_phys_avail(unsigned long paddr);
 
 /*
-- 
1.8.1.2

Re: [PATCH 09/10] sparc64: Add basic validations to {pud,pmd}_bad().

[Sam Ravnborg]

On Wed, Apr 30, 2014 at 05:55:56PM -0400, David Miller wrote:
> 
> Instead of returning false we should at least check the most basic
> things, otherwise page table corruptions will be very difficult to
> debug.
> 
> PMD and PTE tables are of size PAGE_SIZE, so none of the sub-PAGE_SIZE
> bits should be set.
> 
> We also complement this with a check that the physical address the
> pud/pmd points to is valid memory.
> 
> PowerPC was used as a guide while implementating this.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  arch/sparc/include/asm/pgtable_64.h | 46 +++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
> index ff97960..fd4dbdd 100644
> --- a/arch/sparc/include/asm/pgtable_64.h
> +++ b/arch/sparc/include/asm/pgtable_64.h
> @@ -71,6 +71,23 @@
>  
>  #include <linux/sched.h>
>  
> +extern unsigned long sparc64_valid_addr_bitmap[];
> +
> +/* Needs to be defined here and not in linux/mm.h, as it is arch dependent */
> +static inline bool __kern_addr_valid(unsigned long paddr)
It would had been self-documenting if the type is changed to phys_addr_t here.
But I see that we do not use phys_addr_t much (only one place) in sparc code today.

> +{
> +	if ((paddr >> MAX_PHYS_ADDRESS_BITS) != 0UL)
> +		return false;
> +	return test_bit(paddr >> 22, sparc64_valid_addr_bitmap);
> +}
I assume it is a coincidence that we have the same block size
in sparc64_valid_addr_bitmapi (22) as we have for REAL_HPAGE_SHIFT.

	Sam

Re: [PATCH 09/10] sparc64: Add basic validations to {pud,pmd}_bad().

[David Miller]

From: Sam Ravnborg <sam@ravnborg.org>
Date: Thu, 1 May 2014 18:17:02 +0200

> I assume it is a coincidence that we have the same block size
> in sparc64_valid_addr_bitmapi (22) as we have for REAL_HPAGE_SHIFT.

There is no relation.

I should use a macro for this, for sure.

The short story is that I tried to find a value that tries to be
finely grained enough to handle the smallest SIMM that could be
installed, but no moreso in order to not use more memory than
needed for the bitmap.

There are reasons to revisit this choice too, as the increase to
47 bit physical address support exploded the size of some of these
tables.

Thanks.

Re: [PATCH 09/10] sparc64: Add basic validations to {pud,pmd}_bad().

[David Miller]

From: Sam Ravnborg <sam@ravnborg.org>
Date: Thu, 1 May 2014 18:17:02 +0200

> It would had been self-documenting if the type is changed to
> phys_addr_t here.  But I see that we do not use phys_addr_t much
> (only one place) in sparc code today.

I wouldn't be against such a cleanup.

Code has been using "unsigned long" since the dawn of time, most of
this stuff was written before phys_addr_t got added.  And it was added
to handle 32-bit machines with larger than 32-bit physical addresses.

Technically sparc32 could theoretically need it, as it supports 36-bit
physical address in the srmmu PTEs.  But actually in practice it does
not, as only I/O devices get mapped into ares using those high bits.