Re: [PATCH v2 06/11] MIPS: O32/32-bit: Fix bug which can cause incorrect system call restarts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 23, 2014 at 02:40:11PM +0100, Alex Smith wrote:
> From: Alex Smith <alex.smith@xxxxxxxxxx>
> 
> On 32-bit/O32, pt_regs has a padding area at the beginning into which the
> syscall arguments passed via the user stack are copied. 4 arguments
> totalling 16 bytes are copied to offset 16 bytes into this area, however
> the area is only 24 bytes long. This means the last 2 arguments overwrite
> pt_regs->regs[{0,1}].
> 
> If a syscall function returns an error, handle_sys stores the original
> syscall number in pt_regs->regs[0] for syscall restart. signal.c checks
> whether regs[0] is non-zero, if it is it will check whether the syscall
> return value is one of the ERESTART* codes to see if it must be
> restarted.
> 
> Should a syscall be made that results in a non-zero value being copied
> off the user stack into regs[0], and then returns a positive (non-error)
> value that matches one of the ERESTART* error codes, this can be mistaken
> for requiring a syscall restart.
> 
> While the possibility for this to occur has always existed, it is made
> much more likely to occur by commit 46e12c07b3b9 ("MIPS: O32 / 32-bit:
> Always copy 4 stack arguments."), since now every syscall will copy 4
> arguments and overwrite regs[0], rather than just those with 7 or 8
> arguments.
> 
> Since that commit, booting Debian under a 32-bit MIPS kernel almost
> always results in a hang early in boot, due to a wait4 syscall returning
> a PID that matches one of the ERESTART* codes, which then causes an
> incorrect restart of the syscall.
> 
> The problem is fixed by increasing the size of the padding area so that
> arguments copied off the stack will not overwrite pt_regs->regs[{0,1}].
> 
> Signed-off-by: Alex Smith <alex.smith@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v3.13+
> ---
> Changes in v2:
>  - Rebase on current upstream.
>  - Split comment change into a separate commit.
> 
> I've rebased this patch on top of current mips-for-linux-next. However,
> for it to be applied to stable it needs an additional change to the
> PT_PADSLOT* definitions in arch/mips/kernel/smtc-asm.S to account for
> the changed pt_regs offsets. This file no longer exists since SMTC has
> been dropped.
> 
> I'm not sure what the correct way to deal with this is - can an
> alternate version of the patch be submitted for stable?
> ---
>  arch/mips/include/asm/ptrace.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
> index 7e6e682..c301fa9 100644
> --- a/arch/mips/include/asm/ptrace.h
> +++ b/arch/mips/include/asm/ptrace.h
> @@ -23,7 +23,7 @@
>  struct pt_regs {
>  #ifdef CONFIG_32BIT
>  	/* Pad bytes for argument save space on the stack. */
> -	unsigned long pad0[6];
> +	unsigned long pad0[8];
>  #endif
>  
>  	/* Saved main processor registers. */

This patch looks fine to me, and I confirm it fixes a problem. Without
this patch, I am not able to boot a Debian user land on a 32-bit system.
It's a regression, so I think it should be merged as soon as possible,
even if the other patches in this series are merged later.

Reviewed-by: Aurelien Jarno <aurelien@xxxxxxxxxxx>
Tested-by: Aurelien Jarno <aurelien@xxxxxxxxxxx>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@xxxxxxxxxxx                 http://www.aurel32.net
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]