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
