diff mbox series

arm64/mm: Sanity check PTE address before runtime P4D/PUD folding

Message ID 20241101155800.3917462-2-ardb+git@google.com (mailing list archive)
State New
Headers show
Series arm64/mm: Sanity check PTE address before runtime P4D/PUD folding | expand

Commit Message

Ard Biesheuvel Nov. 1, 2024, 3:58 p.m. UTC 
From: Ard Biesheuvel <ardb@kernel.org>

The runtime P4D/PUD folding logic assumes that the respective pgd_t* and
p4d_t* arguments are pointers into actual page tables that are part of
the hierarchy being operated on.

This may not always be the case, and we have been bitten once by this
already [0], where the argument was actually a stack variable, and in
this case, the logic does not work at all.

So let's add a VM_BUG_ON() for each case, to ensure that the address of
the provided page table entry is consistent with the address being
translated: for user space addresses, only index [0] is valid, given
that all other entries translate to addresses that are out of range. The
same applies to kernel address, but in reverse; only the entry at the
very top of the page table should be addressable when the level in
question is being folded at runtime.

So after subtracting the sign bit (-1 or 0) of the address from the
index, the resulting value should be 0x0 modulo PTRS_PER_P?D.

[0] https://lore.kernel.org/all/20240725090345.28461-1-will@kernel.org/T/#u

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/pgtable.h | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Catalin Marinas Nov. 4, 2024, 1:50 p.m. UTC | #1 
On Fri, Nov 01, 2024 at 04:58:01PM +0100, Ard Biesheuvel wrote:
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index dd5dcf7ae056..0d729adf894c 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -740,6 +740,11 @@ static inline bool pud_table(pud_t pud) { return true; }
>  				 PUD_TYPE_TABLE)
>  #endif
>  
> +static inline long sign_of(unsigned long addr)
> +{
> +	return (int)(addr >> 24) >> 31L; // bit 55 is the sign bit
> +}

That's a pretty generic name that trickles into the core code. It should
be renamed to something that suggests arm64 addresses (and maybe some
underscores to imply private). Also, this assumes untagged addresses but
I haven't checked whether that's the case for all call sites.

>  extern pgd_t init_pg_dir[];
>  extern pgd_t init_pg_end[];
>  extern pgd_t swapper_pg_dir[];
> @@ -932,6 +937,8 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
>  
>  static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
>  {
> +	VM_BUG_ON(((u64)p4dp / sizeof(p4d_t) - sign_of(addr)) % PTRS_PER_P4D);
> +
>  	return (pud_t *)PTR_ALIGN_DOWN(p4dp, PAGE_SIZE) + pud_index(addr);
>  }

I think I get it but please add a comment in the code, otherwise in a
week time I'll wonder what this is.

Even better if it could be written in a less cryptic way ;).
Ard Biesheuvel Nov. 4, 2024, 1:57 p.m. UTC | #2 
On Mon, 4 Nov 2024 at 14:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Nov 01, 2024 at 04:58:01PM +0100, Ard Biesheuvel wrote:
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index dd5dcf7ae056..0d729adf894c 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -740,6 +740,11 @@ static inline bool pud_table(pud_t pud) { return true; }
> >                                PUD_TYPE_TABLE)
> >  #endif
> >
> > +static inline long sign_of(unsigned long addr)
> > +{
> > +     return (int)(addr >> 24) >> 31L; // bit 55 is the sign bit
> > +}
>
> That's a pretty generic name that trickles into the core code. It should
> be renamed to something that suggests arm64 addresses (and maybe some
> underscores to imply private).

Fair enough.

> Also, this assumes untagged addresses but
> I haven't checked whether that's the case for all call sites.
>

Isn't bit #55 always guaranteed to be the sign bit?

> >  extern pgd_t init_pg_dir[];
> >  extern pgd_t init_pg_end[];
> >  extern pgd_t swapper_pg_dir[];
> > @@ -932,6 +937,8 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
> >
> >  static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
> >  {
> > +     VM_BUG_ON(((u64)p4dp / sizeof(p4d_t) - sign_of(addr)) % PTRS_PER_P4D);
> > +
> >       return (pud_t *)PTR_ALIGN_DOWN(p4dp, PAGE_SIZE) + pud_index(addr);
> >  }
>
> I think I get it but please add a comment in the code, otherwise in a
> week time I'll wonder what this is.
>
> Even better if it could be written in a less cryptic way ;).
>

Yeah, I tried to keep it concise so the code for the VM_BUG_ON()
doesn't bloat the function.
Catalin Marinas Nov. 4, 2024, 4:02 p.m. UTC | #3 
On Mon, Nov 04, 2024 at 02:57:21PM +0100, Ard Biesheuvel wrote:
> On Mon, 4 Nov 2024 at 14:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Nov 01, 2024 at 04:58:01PM +0100, Ard Biesheuvel wrote:
> > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > > index dd5dcf7ae056..0d729adf894c 100644
> > > --- a/arch/arm64/include/asm/pgtable.h
> > > +++ b/arch/arm64/include/asm/pgtable.h
> > > @@ -740,6 +740,11 @@ static inline bool pud_table(pud_t pud) { return true; }
> > >                                PUD_TYPE_TABLE)
> > >  #endif
> > >
> > > +static inline long sign_of(unsigned long addr)
> > > +{
> > > +     return (int)(addr >> 24) >> 31L; // bit 55 is the sign bit
> > > +}
> >
> > That's a pretty generic name that trickles into the core code. It should
> > be renamed to something that suggests arm64 addresses (and maybe some
> > underscores to imply private).
> 
> Fair enough.
> 
> > Also, this assumes untagged addresses but
> > I haven't checked whether that's the case for all call sites.
> 
> Isn't bit #55 always guaranteed to be the sign bit?

Ah, true, we have the cast to (int) which removes any tag bits.

Why's the 31L? Does it affect the type? (it saves me from having to dig
out the C standard).

> > >  extern pgd_t init_pg_dir[];
> > >  extern pgd_t init_pg_end[];
> > >  extern pgd_t swapper_pg_dir[];
> > > @@ -932,6 +937,8 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
> > >
> > >  static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
> > >  {
> > > +     VM_BUG_ON(((u64)p4dp / sizeof(p4d_t) - sign_of(addr)) % PTRS_PER_P4D);
> > > +
> > >       return (pud_t *)PTR_ALIGN_DOWN(p4dp, PAGE_SIZE) + pud_index(addr);
> > >  }
> >
> > I think I get it but please add a comment in the code, otherwise in a
> > week time I'll wonder what this is.
> >
> > Even better if it could be written in a less cryptic way ;).
> >
> 
> Yeah, I tried to keep it concise so the code for the VM_BUG_ON()
> doesn't bloat the function.

Fine but add a comment along the lines of the commit log.
Ard Biesheuvel Nov. 4, 2024, 4:43 p.m. UTC | #4 
On Mon, 4 Nov 2024 at 17:02, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Mon, Nov 04, 2024 at 02:57:21PM +0100, Ard Biesheuvel wrote:
> > On Mon, 4 Nov 2024 at 14:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Fri, Nov 01, 2024 at 04:58:01PM +0100, Ard Biesheuvel wrote:
> > > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > > > index dd5dcf7ae056..0d729adf894c 100644
> > > > --- a/arch/arm64/include/asm/pgtable.h
> > > > +++ b/arch/arm64/include/asm/pgtable.h
> > > > @@ -740,6 +740,11 @@ static inline bool pud_table(pud_t pud) { return true; }
> > > >                                PUD_TYPE_TABLE)
> > > >  #endif
> > > >
> > > > +static inline long sign_of(unsigned long addr)
> > > > +{
> > > > +     return (int)(addr >> 24) >> 31L; // bit 55 is the sign bit
> > > > +}
> > >
> > > That's a pretty generic name that trickles into the core code. It should
> > > be renamed to something that suggests arm64 addresses (and maybe some
> > > underscores to imply private).
> >
> > Fair enough.
> >
> > > Also, this assumes untagged addresses but
> > > I haven't checked whether that's the case for all call sites.
> >
> > Isn't bit #55 always guaranteed to be the sign bit?
>
> Ah, true, we have the cast to (int) which removes any tag bits.
>
> Why's the 31L? Does it affect the type? (it saves me from having to dig
> out the C standard).
>

Turns out that it doesn't - I'll drop the L suffix.

> > > >  extern pgd_t init_pg_dir[];
> > > >  extern pgd_t init_pg_end[];
> > > >  extern pgd_t swapper_pg_dir[];
> > > > @@ -932,6 +937,8 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
> > > >
> > > >  static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
> > > >  {
> > > > +     VM_BUG_ON(((u64)p4dp / sizeof(p4d_t) - sign_of(addr)) % PTRS_PER_P4D);
> > > > +
> > > >       return (pud_t *)PTR_ALIGN_DOWN(p4dp, PAGE_SIZE) + pud_index(addr);
> > > >  }
> > >
> > > I think I get it but please add a comment in the code, otherwise in a
> > > week time I'll wonder what this is.
> > >
> > > Even better if it could be written in a less cryptic way ;).
> > >
> >
> > Yeah, I tried to keep it concise so the code for the VM_BUG_ON()
> > doesn't bloat the function.
>
> Fine but add a comment along the lines of the commit log.
>

OK
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index dd5dcf7ae056..0d729adf894c 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -740,6 +740,11 @@  static inline bool pud_table(pud_t pud) { return true; }
 				 PUD_TYPE_TABLE)
 #endif
 
+static inline long sign_of(unsigned long addr)
+{
+	return (int)(addr >> 24) >> 31L; // bit 55 is the sign bit
+}
+
 extern pgd_t init_pg_dir[];
 extern pgd_t init_pg_end[];
 extern pgd_t swapper_pg_dir[];
@@ -932,6 +937,8 @@  static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
 
 static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
 {
+	VM_BUG_ON(((u64)p4dp / sizeof(p4d_t) - sign_of(addr)) % PTRS_PER_P4D);
+
 	return (pud_t *)PTR_ALIGN_DOWN(p4dp, PAGE_SIZE) + pud_index(addr);
 }
 
@@ -1056,6 +1063,8 @@  static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
 
 static inline p4d_t *pgd_to_folded_p4d(pgd_t *pgdp, unsigned long addr)
 {
+	VM_BUG_ON(((u64)pgdp / sizeof(pgd_t) - sign_of(addr)) % PTRS_PER_PGD);
+
 	return (p4d_t *)PTR_ALIGN_DOWN(pgdp, PAGE_SIZE) + p4d_index(addr);
 }