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

[Ard Biesheuvel]

On Mon, 4 Nov 2024 at 17:02, Catalin Marinas <catalin.marinas at 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 at 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