Hi,
On Mon, Dec 11, 2023 at 06:18:17PM +0000, Catalin Marinas wrote:
> On Fri, Nov 24, 2023 at 04:34:57PM +0000, Joey Gouly wrote:
> > @@ -497,6 +498,23 @@ static void do_bad_area(unsigned long far, unsigned long esr,
> > #define VM_FAULT_BADMAP ((__force vm_fault_t)0x010000)
> > #define VM_FAULT_BADACCESS ((__force vm_fault_t)0x020000)
> >
> > +static bool fault_from_pkey(unsigned long esr, struct vm_area_struct *vma,
> > + unsigned int mm_flags)
> > +{
> > + unsigned long iss2 = ESR_ELx_ISS2(esr);
> > +
> > + if (!arch_pkeys_enabled())
> > + return false;
> > +
> > + if (iss2 & ESR_ELx_Overlay)
> > + return true;
> > +
> > + return !arch_vma_access_permitted(vma,
> > + mm_flags & FAULT_FLAG_WRITE,
> > + mm_flags & FAULT_FLAG_INSTRUCTION,
> > + mm_flags & FAULT_FLAG_REMOTE);
> > +}
>
> Do we actually need this additional arch_vma_access_permitted() check?
> The ESR should tell us if it was a POE fault. Permission overlay faults
> have priority over the base permission faults, so we'd not need to fall
> back to this additional checks. Well, see below, we could do something
> slightly smarter here.
We want this here as it follows other arch's which will fail with a pkey fault
even if the page isn't actually mapped. If the paged isn't mapped we'd get a
translation fault, but since we know the type of access and have the pkey in
the VMA we check it here.
>
> I can see x86 and powerpc have similar checks (though at a different
> point under the mmap lock) but I'm not familiar with their exception
> model, exception priorities.
>
> > +
> > static vm_fault_t __do_page_fault(struct mm_struct *mm,
> > struct vm_area_struct *vma, unsigned long addr,
> > unsigned int mm_flags, unsigned long vm_flags,
> > @@ -688,9 +706,29 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> > * Something tried to access memory that isn't in our memory
> > * map.
> > */
> > - arm64_force_sig_fault(SIGSEGV,
> > - fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : SEGV_MAPERR,
> > - far, inf->name);
> > + int fault_kind;
> > + /*
> > + * The pkey value that we return to userspace can be different
> > + * from the pkey that caused the fault.
> > + *
> > + * 1. T1 : mprotect_key(foo, PAGE_SIZE, pkey=4);
> > + * 2. T1 : set AMR to deny access to pkey=4, touches, page
> > + * 3. T1 : faults...
> > + * 4. T2: mprotect_key(foo, PAGE_SIZE, pkey=5);
> > + * 5. T1 : enters fault handler, takes mmap_lock, etc...
> > + * 6. T1 : reaches here, sees vma_pkey(vma)=5, when we really
> > + * faulted on a pte with its pkey=4.
> > + */
> > + int pkey = vma_pkey(vma);
>
> Other than the vma_pkey() race, I'm more worried about the vma
> completely disappearing, e.g. munmap() in another thread. We end up
> dereferencing a free pointer here. We need to do this under the mmap
> lock.
>
> Since we need to do this check under the mmap lock, we could add an
> additional check to see if the pkey fault we got was a racy and just
> restart the instruction if it no longer faults according to
> por_el0_allows_pkey(). However, the code below is too late in the fault
> handling to be able to do much other than SIGSEGV.
After discussing in person, I agree with the assesment that this is the wrong
place to do the check, and after looking at the x86 arch code, I see how
they're doing it earlier.
Thanks,
Joey
