Re: [PATCH] Fixed Microblaze syscall error recovery for invalid syscall IDs.

[Michal Simek <monstr@monstr.eu>]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

On 02/23/2015 04:35 PM, jamie.garside@york.ac.uk wrote:
> From: Jamie Garside <jamie.garside@york.ac.uk>
> 
> This patch fixes two bugs in the Microblaze syscall trap handler when an invalid
> syscall ID is used.
> 
> First, the range check on line 351 only checks for syscall IDs greater than
> __NR_syscalls. A negative syscall ID (either passed to `syscall()` or as returned
> by `do_syscall_trace_enter()` on error) will still satisfy this test and cause
> the Linux kernel to access an invalid memory location and cause a kernel oops.
> This has been fixed by also checking for r12 < 0.
> 
> Secondly, the current error recovery at line 378 returns using the wrong register
> (r15 instead of r14) and does not restore the previous stack state. This has been
> fixed by invoking `ret_from_trap` on error, setting r3 to `-ENOSYS`, similar to
> what would happen when calling a valid syscall.
> 
> Signed-off-by: Jamie Garside <jamie.garside@york.ac.uk>
> ---
>  arch/microblaze/kernel/entry.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/microblaze/kernel/entry.S b/arch/microblaze/kernel/entry.S
> index 0536bc0..101f13c 100644
> --- a/arch/microblaze/kernel/entry.S
> +++ b/arch/microblaze/kernel/entry.S
> @@ -348,8 +348,9 @@ C_ENTRY(_user_exception):
>   * The LP register should point to the location where the called function
>   * should return.  [note that MAKE_SYS_CALL uses label 1] */
>  	/* See if the system call number is valid */
> +        blti    r12, 5f
>  	addi	r11, r12, -__NR_syscalls;
> -	bgei	r11,5f;
> +	bgei	r11, 5f;
>  	/* Figure out which function to use for this system call.  */
>  	/* Note Microblaze barrel shift is optional, so don't rely on it */
>  	add	r12, r12, r12;			/* convert num -> ptr */
> @@ -375,7 +376,7 @@ C_ENTRY(_user_exception):
>  
>  	/* The syscall number is invalid, return an error.  */
>  5:
> -	rtsd	r15, 8;		/* looks like a normal subroutine return */
> +        braid   ret_from_trap
>  	addi	r3, r0, -ENOSYS;
>  
>  /* Entry point used to return from a syscall/trap */
> 

Looks reasonable and I have also tested it.
I have fixed subject, coding style and I have create one more
patch to fix that coding style issue.
The patches should be in linux-next.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]