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

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

On 12/04/2019 12:55, Will Deacon wrote:
> [+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? ;)
> 

Right! ;) I will revert back to AARCH32_VECTORS_BASE to keep it consistent.

>>>> -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?
> 

We could if it make sense for arm to change it accordingly, but for the moment I
would prefer to maintain consistency.

> [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.
> 

Agreed.

>>>> +	/*
>>>> +	 * 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.

Ok, I am going to re-post the set with the proposed changes.

> 
> Will
> 

-- 
Regards,
Vincenzo

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