Re: [PATCH v3 1/4] arm64: compat: Alloc separate pages for vectors and sigpage

[Will Deacon <will.deacon@arm.com>]

[+rmk]

On Fri, Apr 12, 2019 at 12:08:30PM +0100, Vincenzo Frascino wrote:
> On 10/04/2019 19:10, Will Deacon wrote:
> > On Tue, Apr 02, 2019 at 05:27:54PM +0100, Vincenzo Frascino wrote:
> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> >> index 5d9ce62bdebd..07c873fce961 100644
> >> --- a/arch/arm64/include/asm/processor.h
> >> +++ b/arch/arm64/include/asm/processor.h
> >> @@ -78,9 +78,9 @@
> >>  #endif /* CONFIG_ARM64_FORCE_52BIT */
> >>  
> >>  #ifdef CONFIG_COMPAT
> >> -#define AARCH32_VECTORS_BASE	0xffff0000
> >> +#define AARCH32_KUSER_BASE	0xffff0000
> >>  #define STACK_TOP		(test_thread_flag(TIF_32BIT) ? \
> >> -				AARCH32_VECTORS_BASE : STACK_TOP_MAX)
> >> +				AARCH32_KUSER_BASE : STACK_TOP_MAX)
> >>  #else
> >>  #define STACK_TOP		STACK_TOP_MAX
> >>  #endif /* CONFIG_COMPAT */
> >> diff --git a/arch/arm64/include/asm/signal32.h b/arch/arm64/include/asm/signal32.h
> >> index 81abea0b7650..58e288aaf0ba 100644
> >> --- a/arch/arm64/include/asm/signal32.h
> >> +++ b/arch/arm64/include/asm/signal32.h
> >> @@ -20,8 +20,6 @@
> >>  #ifdef CONFIG_COMPAT
> >>  #include <linux/compat.h>
> >>  
> >> -#define AARCH32_KERN_SIGRET_CODE_OFFSET	0x500
> >> -
> >>  int compat_setup_frame(int usig, struct ksignal *ksig, sigset_t *set,
> >>  		       struct pt_regs *regs);
> >>  int compat_setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
> >> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> >> index cb7800acd19f..3846a1b710b5 100644
> >> --- a/arch/arm64/kernel/signal32.c
> >> +++ b/arch/arm64/kernel/signal32.c
> >> @@ -379,6 +379,7 @@ static void compat_setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> >>  	compat_ulong_t retcode;
> >>  	compat_ulong_t spsr = regs->pstate & ~(PSR_f | PSR_AA32_E_BIT);
> >>  	int thumb;
> >> +	void *sigreturn_base;
> >>  
> >>  	/* Check if the handler is written for ARM or Thumb */
> >>  	thumb = handler & 1;
> >> @@ -399,12 +400,12 @@ static void compat_setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> >>  	} else {
> >>  		/* Set up sigreturn pointer */
> >>  		unsigned int idx = thumb << 1;
> >> +		sigreturn_base = current->mm->context.vdso;
> >>  
> >>  		if (ka->sa.sa_flags & SA_SIGINFO)
> >>  			idx += 3;
> >>  
> >> -		retcode = AARCH32_VECTORS_BASE +
> >> -			  AARCH32_KERN_SIGRET_CODE_OFFSET +
> >> +		retcode = ptr_to_compat(sigreturn_base) +
> >>  			  (idx << 2) + thumb;
> >>  	}
> >>  
> >> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> >> index 2d419006ad43..16f8fce5c501 100644
> >> --- a/arch/arm64/kernel/vdso.c
> >> +++ b/arch/arm64/kernel/vdso.c
> >> @@ -1,5 +1,7 @@
> >>  /*
> >> - * VDSO implementation for AArch64 and vector page setup for AArch32.
> >> + * VDSO implementation for AArch64 and for AArch32:
> >> + * AArch64: vDSO implementation contains pages setup and data page update.
> >> + * AArch32: vDSO implementation contains sigreturn and kuser pages setup.
> >>   *
> >>   * Copyright (C) 2012 ARM Limited
> >>   *
> >> @@ -53,61 +55,126 @@ struct vdso_data *vdso_data = &vdso_data_store.data;
> >>  /*
> >>   * Create and map the vectors page for AArch32 tasks.
> >>   */
> >> -static struct page *vectors_page[1] __ro_after_init;
> >> +/*
> >> + * aarch32_vdso_pages:
> >> + * 0 - kuser helpers
> >> + * 1 - sigreturn code
> >> + */
> >> +#define C_VECTORS	0
> > 
> > C_KUSER might make more sense, and is consistent with AARCH32_KUSER_BASE.
> >
> 
> C_VECTORS seems consistent with the name of the page it refers to.

Ok, then why change AARCH32_VECTORS_BASE? ;)

> >> -int aarch32_setup_vectors_page(struct linux_binprm *bprm, int uses_interp)
> >> +static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
> >>  {
> >> -	struct mm_struct *mm = current->mm;
> >> -	unsigned long addr = AARCH32_VECTORS_BASE;
> >> -	static const struct vm_special_mapping spec = {
> >> -		.name	= "[vectors]",
> >> -		.pages	= vectors_page,
> >> +	void *ret;
> >> +
> >> +	/* The kuser helpers must be mapped at the ABI-defined high address */
> >> +	ret = _install_special_mapping(mm, AARCH32_KUSER_BASE, PAGE_SIZE,
> >> +				       VM_READ | VM_EXEC |
> >> +				       VM_MAYREAD | VM_MAYEXEC,
> > 
> > How come you don't need VM_MAYWRITE here...
> > 
> 
> This is to keep it consistent with what the arm (32 bit) implementation does.
> 
> My understanding is that the kuser code is executed in user mode for efficiency
> reasons but it is too close to the kernel to be implemented in user libraries
> and that the kernel can change its internal implementation from version to
> version as far as it guarantees the "interface" (entry points and results).
> Based on this gdb should not need to put a breakpoint inside the kuser helpers code.

Hmm, but couldn't you apply the same reasoning to the sigpage?

[also, talking to Russell, he makes the very good point that you can't CoW
 the page containing the vectors because that would give userspace control
 over the vectors themselves!]

> And if we consider as well that the fixed address nature of the helpers could be
> used from ROP authors during the creation of exploits probably we want to
> prevent gdb to set a breakpoint there hence the proposed patch does not contain
> VM_MAYWRITE.

I'm not sure I buy the ROP angle... you need to GUP the thing to get the
write to happen. Maybe you could do it with a futex or something, but I'm
also not sure that's really a viable attack.

> I had a look to arm implementation and it seems the it defines the vector page
> as READONLY_EXEC and there is no VM_MAYWRITE in the vm_flags.
> 
> I could extend the comment accordingly.

I initially thought that the gate VMA would mean that VM_MAYWRITE was
implicit for GUP, but actually that's handled explicitly in get_gate_page():

	/* user gate pages are read-only */
	if (gup_flags & FOLL_WRITE)
		return -EFAULT;

so yes, I agree with you that this is consistent with 32-bit. What I'm not
sure about is why we need to CoW the sigpage. But I suppose being compatible
with 32-bit is the aim of the game, so this is all moot.

> >> +	/*
> >> +	 * VM_MAYWRITE is required to allow gdb to Copy-on-Write and
> >> +	 * set breakpoints.
> >> +	 */
> >>  	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
> >> -				       VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYEXEC,
> >> -				       &spec);
> >> +				       VM_READ | VM_EXEC | VM_MAYREAD |
> >> +				       VM_MAYWRITE | VM_MAYEXEC,
> >> +				       &aarch32_vdso_spec[C_SIGPAGE]);
> > 
> > ... but you introduce it here?Also, shouldn't this be a separate change
> > so it can be treated as a fix?
> 
> This is again to keep it consistent with what the arm implementation does.
> 
> Since the separation (vectors/sigpage) has been added in this patch, I am not
> sure on how we can treat this change as a separate patch, at least based on what
> I mentioned above. Could you please explain?

Sorry, I was getting ahead of myself with that. If we need to add
VM_MAYWRITE to the compat kuser helpers (which we could safely do on
64-bit), then we could do it as a fix. However, we don't need to do that
do please ignore my comment above.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel