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

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

Hi Will,

thank you for your review.

On 10/04/2019 19:10, Will Deacon wrote:
> On Tue, Apr 02, 2019 at 05:27:54PM +0100, Vincenzo Frascino wrote:
>> In the current implementation AArch32 installs a special page called
>> "[vectors]" that contains sigreturn trampolines and kuser helpers,
> 
> Doesn't make sense. How about:
> 
> "For AArch32 tasks, we install a special "[vectors]" page that contains
> the sigreturn trampolines and kuser helpers."
> 
>> and this is done at fixed address specified by the kuser helpers ABI.
> 
> which is mapped at a fixed address...
> 
>> Having sigreturn trampolines and kuser helpers in the same page, makes
>> difficult to maintain compatibility with arm because it makes not
>> possible to disable kuser helpers.
> 
> Replace with:
> 
> "Having the sigreturn trampolines in the same page as the kuser helpers
>  makes it impossible to disable the kuser helpers independently."
> 
>> Address the problem creating separate pages for vectors and sigpage in
>> a similar fashion to what happens today on arm.
> 
> "Follow the Arm implementation, by moving the signal trampolines out of
>  the "[vectors]" page and into their own "[sigpage]"".
> 
>> Change as well the meaning of mm->context.vdso for AArch32 compat since
>> it now points to sigpage and not to vectors anymore in order to make
>> simpler the implementation of the signal handling (the address of
>> sigpage is randomized).
> 
> This is an implementation detail and can be dropped.
> 

Thanks for this, I will rework my patch description in v4.

>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>>  arch/arm64/include/asm/elf.h       |   6 +-
>>  arch/arm64/include/asm/processor.h |   4 +-
>>  arch/arm64/include/asm/signal32.h  |   2 -
>>  arch/arm64/kernel/signal32.c       |   5 +-
>>  arch/arm64/kernel/vdso.c           | 121 ++++++++++++++++++++++-------
>>  5 files changed, 102 insertions(+), 36 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>> index 6adc1a90e7e6..355d120b78cb 100644
>> --- a/arch/arm64/include/asm/elf.h
>> +++ b/arch/arm64/include/asm/elf.h
>> @@ -214,10 +214,10 @@ typedef compat_elf_greg_t		compat_elf_gregset_t[COMPAT_ELF_NGREG];
>>  	set_thread_flag(TIF_32BIT);					\
>>   })
>>  #define COMPAT_ARCH_DLINFO
>> -extern int aarch32_setup_vectors_page(struct linux_binprm *bprm,
>> -				      int uses_interp);
>> +extern int aarch32_setup_additional_pages(struct linux_binprm *bprm,
>> +					  int uses_interp);
>>  #define compat_arch_setup_additional_pages \
>> -					aarch32_setup_vectors_page
>> +					aarch32_setup_additional_pages
>>  
>>  #endif /* CONFIG_COMPAT */
>>  
>> 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.

>> +#define C_SIGPAGE	1
>> +#define C_PAGES		(C_SIGPAGE + 1)
>> +static struct page *aarch32_vdso_pages[C_PAGES] __ro_after_init;
>> +static const struct vm_special_mapping aarch32_vdso_spec[C_PAGES] = {
>> +	{
>> +		/* Must be named [vectors] for compatibility with arm. */
>> +		.name	= "[vectors]",
>> +		.pages	= &aarch32_vdso_pages[C_VECTORS],
>> +	},
>> +	{
>> +		/* Must be named [sigpage] for compatibility with arm. */
>> +		.name	= "[sigpage]",
>> +		.pages	= &aarch32_vdso_pages[C_SIGPAGE],
>> +	},
>> +};
>>  
>> -static int __init alloc_vectors_page(void)
>> +static int __init aarch32_alloc_vdso_pages(void)
> 
> Premature renaming of the function?
> 

I named/renamed everything that refers to aarch32 as "aarch32_" to make it
easier to follow the code flow.

>>  {
>>  	extern char __kuser_helper_start[], __kuser_helper_end[];
>>  	extern char __aarch32_sigret_code_start[], __aarch32_sigret_code_end[];
>>  
>>  	int kuser_sz = __kuser_helper_end - __kuser_helper_start;
>>  	int sigret_sz = __aarch32_sigret_code_end - __aarch32_sigret_code_start;
>> -	unsigned long vpage;
>> +	unsigned long vdso_pages[C_PAGES];
>>  
>> -	vpage = get_zeroed_page(GFP_ATOMIC);
>> +	vdso_pages[C_VECTORS] = get_zeroed_page(GFP_ATOMIC);
>> +	if (!vdso_pages[C_VECTORS])
>> +		return -ENOMEM;
>>  
>> -	if (!vpage)
>> +	vdso_pages[C_SIGPAGE] = get_zeroed_page(GFP_ATOMIC);
>> +	if (!vdso_pages[C_SIGPAGE])
>>  		return -ENOMEM;
> 
> You leak the kuser page if this fails.
> 

Thanks for this, I will definitely fix it in v4.

>>  	/* kuser helpers */
>> -	memcpy((void *)vpage + 0x1000 - kuser_sz, __kuser_helper_start,
>> -		kuser_sz);
>> +	memcpy((void *)(vdso_pages[C_VECTORS] + 0x1000 - kuser_sz),
>> +	       __kuser_helper_start,
>> +	       kuser_sz);
>>  
>>  	/* sigreturn code */
>> -	memcpy((void *)vpage + AARCH32_KERN_SIGRET_CODE_OFFSET,
>> -               __aarch32_sigret_code_start, sigret_sz);
>> +	memcpy((void *)vdso_pages[C_SIGPAGE],
>> +	       __aarch32_sigret_code_start,
>> +	       sigret_sz);
>>  
>> -	flush_icache_range(vpage, vpage + PAGE_SIZE);
>> -	vectors_page[0] = virt_to_page(vpage);
>> +	flush_icache_range(vdso_pages[C_VECTORS],
>> +			   vdso_pages[C_VECTORS] + PAGE_SIZE);
>> +	flush_icache_range(vdso_pages[C_SIGPAGE],
>> +			   vdso_pages[C_SIGPAGE] + PAGE_SIZE);
>> +
>> +	aarch32_vdso_pages[C_VECTORS] = virt_to_page(vdso_pages[C_VECTORS]);
>> +	aarch32_vdso_pages[C_SIGPAGE] = virt_to_page(vdso_pages[C_SIGPAGE]);
>>  
>>  	return 0;
>>  }
>> -arch_initcall(alloc_vectors_page);
>> +arch_initcall(aarch32_alloc_vdso_pages);
>>  
>> -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.

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

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

> 
> Will
> 

-- 
Regards,
Vincenzo

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