[PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
(o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
arguments by incrementing both the start argument number and the number
of arguments to fetch. However only the start argument number needs to
be incremented. The number of arguments does not change, they're just
shifted up by one, and in fact the output array is provided by the
caller and is likely only n entries long, so reading more arguments
overflows the output buffer.

In the case of seccomp, this results in it fetching 7 arguments starting
at the 2nd one, which overflows the unsigned long args[6] in
populate_seccomp_data(). This clobbers the $s0 register from
syscall_trace_enter() which __seccomp_phase1_filter() saved onto the
stack, into which syscall_trace_enter() had placed its syscall number
argument. This caused Chromium to crash.

Credit goes to Milko for tracking it down as far as $s0 being clobbered.

Fixes: 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls (o32)")
Reported-by: Milko Leporis <milko.leporis@imgtec.com>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # 3.15-
---
 arch/mips/include/asm/syscall.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index 6499d93ae68d..47bc45a67e9b 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
 	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
 	if ((config_enabled(CONFIG_32BIT) ||
 	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
-	    (regs->regs[2] == __NR_syscall)) {
+	    (regs->regs[2] == __NR_syscall))
 		i++;
-		n++;
-	}
 
 	while (n--)
 		ret |= mips_get_syscall_arg(args++, task, regs, i++);
-- 
2.4.10

[PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
(o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
arguments by incrementing both the start argument number and the number
of arguments to fetch. However only the start argument number needs to
be incremented. The number of arguments does not change, they're just
shifted up by one, and in fact the output array is provided by the
caller and is likely only n entries long, so reading more arguments
overflows the output buffer.

In the case of seccomp, this results in it fetching 7 arguments starting
at the 2nd one, which overflows the unsigned long args[6] in
populate_seccomp_data(). This clobbers the $s0 register from
syscall_trace_enter() which __seccomp_phase1_filter() saved onto the
stack, into which syscall_trace_enter() had placed its syscall number
argument. This caused Chromium to crash.

Credit goes to Milko for tracking it down as far as $s0 being clobbered.

Fixes: 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls (o32)")
Reported-by: Milko Leporis <milko.leporis@imgtec.com>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # 3.15-
---
 arch/mips/include/asm/syscall.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index 6499d93ae68d..47bc45a67e9b 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
 	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
 	if ((config_enabled(CONFIG_32BIT) ||
 	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
-	    (regs->regs[2] == __NR_syscall)) {
+	    (regs->regs[2] == __NR_syscall))
 		i++;
-		n++;
-	}
 
 	while (n--)
 		ret |= mips_get_syscall_arg(args++, task, regs, i++);
-- 
2.4.10

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[Maciej W. Rozycki]

On Mon, 25 Jan 2016, James Hogan wrote:

> Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
> (o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
> arguments by incrementing both the start argument number and the number
> of arguments to fetch. However only the start argument number needs to
> be incremented. The number of arguments does not change, they're just
> shifted up by one, and in fact the output array is provided by the
> caller and is likely only n entries long, so reading more arguments
> overflows the output buffer.
[...]
> diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> index 6499d93ae68d..47bc45a67e9b 100644
> --- a/arch/mips/include/asm/syscall.h
> +++ b/arch/mips/include/asm/syscall.h
> @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
>  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
>  	if ((config_enabled(CONFIG_32BIT) ||
>  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> -	    (regs->regs[2] == __NR_syscall)) {
> +	    (regs->regs[2] == __NR_syscall))
>  		i++;
> -		n++;
> -	}
>  
>  	while (n--)
>  		ret |= mips_get_syscall_arg(args++, task, regs, i++);

 What I think it really needs to do is to *decrease* the number of 
arguments, as we're throwing the syscall number away as not an argument to 
itself.  So this looks like a typo to me, the expression was meant to be 
`n--' rather than `n++'.  With the number of arguments unchanged, as in 
your proposed change, we're still reaching one word too far.

  Maciej

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[Maciej W. Rozycki]

On Mon, 25 Jan 2016, James Hogan wrote:

> Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
> (o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
> arguments by incrementing both the start argument number and the number
> of arguments to fetch. However only the start argument number needs to
> be incremented. The number of arguments does not change, they're just
> shifted up by one, and in fact the output array is provided by the
> caller and is likely only n entries long, so reading more arguments
> overflows the output buffer.
[...]
> diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> index 6499d93ae68d..47bc45a67e9b 100644
> --- a/arch/mips/include/asm/syscall.h
> +++ b/arch/mips/include/asm/syscall.h
> @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
>  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
>  	if ((config_enabled(CONFIG_32BIT) ||
>  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> -	    (regs->regs[2] == __NR_syscall)) {
> +	    (regs->regs[2] == __NR_syscall))
>  		i++;
> -		n++;
> -	}
>  
>  	while (n--)
>  		ret |= mips_get_syscall_arg(args++, task, regs, i++);

 What I think it really needs to do is to *decrease* the number of 
arguments, as we're throwing the syscall number away as not an argument to 
itself.  So this looks like a typo to me, the expression was meant to be 
`n--' rather than `n++'.  With the number of arguments unchanged, as in 
your proposed change, we're still reaching one word too far.

  Maciej

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

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

Hi Maciej,

On Sun, Jan 31, 2016 at 11:33:30PM +0000, Maciej W. Rozycki wrote:
> On Mon, 25 Jan 2016, James Hogan wrote:
> 
> > Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
> > (o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
> > arguments by incrementing both the start argument number and the number
> > of arguments to fetch. However only the start argument number needs to
> > be incremented. The number of arguments does not change, they're just
> > shifted up by one, and in fact the output array is provided by the
> > caller and is likely only n entries long, so reading more arguments
> > overflows the output buffer.
> [...]
> > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > index 6499d93ae68d..47bc45a67e9b 100644
> > --- a/arch/mips/include/asm/syscall.h
> > +++ b/arch/mips/include/asm/syscall.h
> > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> >  	if ((config_enabled(CONFIG_32BIT) ||
> >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > -	    (regs->regs[2] == __NR_syscall)) {
> > +	    (regs->regs[2] == __NR_syscall))
> >  		i++;
> > -		n++;
> > -	}
> >  
> >  	while (n--)
> >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> 
>  What I think it really needs to do is to *decrease* the number of 
> arguments, as we're throwing the syscall number away as not an argument to 
> itself.  So this looks like a typo to me, the expression was meant to be 
> `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> your proposed change, we're still reaching one word too far.

No, the caller asked for n args, thats what it should get. It doesn't
care whether the syscall was indirected or not.

The syscall doesn't have 1 less arg as a result of indirection. E.g.
What about system calls with 6 arguments, and hence 7 arguments
including syscall number argument when redirected? We'd get an
uninitialised 6th arg back when passing n=6.

Note scall32-o32.S sys_syscall shifts 7 arguments starting at a1 (I've
reordered code slightly):
	move	a0, a1				# shift argument registers
	move	a1, a2
	move	a2, a3
	lw	a3, 16(sp)
	lw	t4, 20(sp)
	lw	t5, 24(sp)
	lw	t6, 28(sp)

	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
	sw	a1, PT_R5(sp)			# syscalls expect them there
	sw	a2, PT_R6(sp)
	sw	a3, PT_R7(sp)
	sw	t4, 16(sp)
	sw	t5, 20(sp)
	sw	t6, 24(sp)

So it takes args 0..2 from a1..a3, and 3..6 from stack.

I presume the handling of 7th arg after syscall number argument is for
fadvise64_64 and sync_file_range, which had badly designed arguments
(odd 64bit args requiring a word of padding).

Cheers
James

> 
>   Maciej

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

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

Hi Maciej,

On Sun, Jan 31, 2016 at 11:33:30PM +0000, Maciej W. Rozycki wrote:
> On Mon, 25 Jan 2016, James Hogan wrote:
> 
> > Since commit 4c21b8fd8f14 ("MIPS: seccomp: Handle indirect system calls
> > (o32)"), syscall_get_arguments() attempts to handle o32 indirect syscall
> > arguments by incrementing both the start argument number and the number
> > of arguments to fetch. However only the start argument number needs to
> > be incremented. The number of arguments does not change, they're just
> > shifted up by one, and in fact the output array is provided by the
> > caller and is likely only n entries long, so reading more arguments
> > overflows the output buffer.
> [...]
> > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > index 6499d93ae68d..47bc45a67e9b 100644
> > --- a/arch/mips/include/asm/syscall.h
> > +++ b/arch/mips/include/asm/syscall.h
> > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> >  	if ((config_enabled(CONFIG_32BIT) ||
> >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > -	    (regs->regs[2] == __NR_syscall)) {
> > +	    (regs->regs[2] == __NR_syscall))
> >  		i++;
> > -		n++;
> > -	}
> >  
> >  	while (n--)
> >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> 
>  What I think it really needs to do is to *decrease* the number of 
> arguments, as we're throwing the syscall number away as not an argument to 
> itself.  So this looks like a typo to me, the expression was meant to be 
> `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> your proposed change, we're still reaching one word too far.

No, the caller asked for n args, thats what it should get. It doesn't
care whether the syscall was indirected or not.

The syscall doesn't have 1 less arg as a result of indirection. E.g.
What about system calls with 6 arguments, and hence 7 arguments
including syscall number argument when redirected? We'd get an
uninitialised 6th arg back when passing n=6.

Note scall32-o32.S sys_syscall shifts 7 arguments starting at a1 (I've
reordered code slightly):
	move	a0, a1				# shift argument registers
	move	a1, a2
	move	a2, a3
	lw	a3, 16(sp)
	lw	t4, 20(sp)
	lw	t5, 24(sp)
	lw	t6, 28(sp)

	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
	sw	a1, PT_R5(sp)			# syscalls expect them there
	sw	a2, PT_R6(sp)
	sw	a3, PT_R7(sp)
	sw	t4, 16(sp)
	sw	t5, 20(sp)
	sw	t6, 24(sp)

So it takes args 0..2 from a1..a3, and 3..6 from stack.

I presume the handling of 7th arg after syscall number argument is for
fadvise64_64 and sync_file_range, which had badly designed arguments
(odd 64bit args requiring a word of padding).

Cheers
James

> 
>   Maciej

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[Maciej W. Rozycki]

On Mon, 1 Feb 2016, James Hogan wrote:

> > > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > > index 6499d93ae68d..47bc45a67e9b 100644
> > > --- a/arch/mips/include/asm/syscall.h
> > > +++ b/arch/mips/include/asm/syscall.h
> > > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> > >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> > >  	if ((config_enabled(CONFIG_32BIT) ||
> > >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > > -	    (regs->regs[2] == __NR_syscall)) {
> > > +	    (regs->regs[2] == __NR_syscall))
> > >  		i++;
> > > -		n++;
> > > -	}
> > >  
> > >  	while (n--)
> > >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> > 
> >  What I think it really needs to do is to *decrease* the number of 
> > arguments, as we're throwing the syscall number away as not an argument to 
> > itself.  So this looks like a typo to me, the expression was meant to be 
> > `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> > your proposed change, we're still reaching one word too far.
> 
> No, the caller asked for n args, thats what it should get. It doesn't
> care whether the syscall was indirected or not.
> 
> The syscall doesn't have 1 less arg as a result of indirection. E.g.
> What about system calls with 6 arguments, and hence 7 arguments
> including syscall number argument when redirected? We'd get an
> uninitialised 6th arg back when passing n=6.

 Do you mean the caller ignores the extra argument holding the number of 
the wrapped syscall in counting syscall arguments?  Where does it take `n' 
from?

> Note scall32-o32.S sys_syscall shifts 7 arguments starting at a1 (I've
> reordered code slightly):
> 	move	a0, a1				# shift argument registers
> 	move	a1, a2
> 	move	a2, a3
> 	lw	a3, 16(sp)
> 	lw	t4, 20(sp)
> 	lw	t5, 24(sp)
> 	lw	t6, 28(sp)
> 
> 	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
> 	sw	a1, PT_R5(sp)			# syscalls expect them there
> 	sw	a2, PT_R6(sp)
> 	sw	a3, PT_R7(sp)
> 	sw	t4, 16(sp)
> 	sw	t5, 20(sp)
> 	sw	t6, 24(sp)
> 
> So it takes args 0..2 from a1..a3, and 3..6 from stack.

 Sure, no need to explain it as far as I'm concerned, I didn't question 
it.

  Maciej

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[Maciej W. Rozycki]

On Mon, 1 Feb 2016, James Hogan wrote:

> > > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > > index 6499d93ae68d..47bc45a67e9b 100644
> > > --- a/arch/mips/include/asm/syscall.h
> > > +++ b/arch/mips/include/asm/syscall.h
> > > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> > >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> > >  	if ((config_enabled(CONFIG_32BIT) ||
> > >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > > -	    (regs->regs[2] == __NR_syscall)) {
> > > +	    (regs->regs[2] == __NR_syscall))
> > >  		i++;
> > > -		n++;
> > > -	}
> > >  
> > >  	while (n--)
> > >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> > 
> >  What I think it really needs to do is to *decrease* the number of 
> > arguments, as we're throwing the syscall number away as not an argument to 
> > itself.  So this looks like a typo to me, the expression was meant to be 
> > `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> > your proposed change, we're still reaching one word too far.
> 
> No, the caller asked for n args, thats what it should get. It doesn't
> care whether the syscall was indirected or not.
> 
> The syscall doesn't have 1 less arg as a result of indirection. E.g.
> What about system calls with 6 arguments, and hence 7 arguments
> including syscall number argument when redirected? We'd get an
> uninitialised 6th arg back when passing n=6.

 Do you mean the caller ignores the extra argument holding the number of 
the wrapped syscall in counting syscall arguments?  Where does it take `n' 
from?

> Note scall32-o32.S sys_syscall shifts 7 arguments starting at a1 (I've
> reordered code slightly):
> 	move	a0, a1				# shift argument registers
> 	move	a1, a2
> 	move	a2, a3
> 	lw	a3, 16(sp)
> 	lw	t4, 20(sp)
> 	lw	t5, 24(sp)
> 	lw	t6, 28(sp)
> 
> 	sw	a0, PT_R4(sp)			# .. and push back a0 - a3, some
> 	sw	a1, PT_R5(sp)			# syscalls expect them there
> 	sw	a2, PT_R6(sp)
> 	sw	a3, PT_R7(sp)
> 	sw	t4, 16(sp)
> 	sw	t5, 20(sp)
> 	sw	t6, 24(sp)
> 
> So it takes args 0..2 from a1..a3, and 3..6 from stack.

 Sure, no need to explain it as far as I'm concerned, I didn't question 
it.

  Maciej

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

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

On Mon, Feb 01, 2016 at 12:50:16PM +0000, Maciej W. Rozycki wrote:
> On Mon, 1 Feb 2016, James Hogan wrote:
> 
> > > > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > > > index 6499d93ae68d..47bc45a67e9b 100644
> > > > --- a/arch/mips/include/asm/syscall.h
> > > > +++ b/arch/mips/include/asm/syscall.h
> > > > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> > > >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> > > >  	if ((config_enabled(CONFIG_32BIT) ||
> > > >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > > > -	    (regs->regs[2] == __NR_syscall)) {
> > > > +	    (regs->regs[2] == __NR_syscall))
> > > >  		i++;
> > > > -		n++;
> > > > -	}
> > > >  
> > > >  	while (n--)
> > > >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> > > 
> > >  What I think it really needs to do is to *decrease* the number of 
> > > arguments, as we're throwing the syscall number away as not an argument to 
> > > itself.  So this looks like a typo to me, the expression was meant to be 
> > > `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> > > your proposed change, we're still reaching one word too far.
> > 
> > No, the caller asked for n args, thats what it should get. It doesn't
> > care whether the syscall was indirected or not.
> > 
> > The syscall doesn't have 1 less arg as a result of indirection. E.g.
> > What about system calls with 6 arguments, and hence 7 arguments
> > including syscall number argument when redirected? We'd get an
> > uninitialised 6th arg back when passing n=6.
> 
>  Do you mean the caller ignores the extra argument holding the number of 
> the wrapped syscall in counting syscall arguments?

No, the caller never sees the extra argument containing syscall number,
because i is incremented, so if caller asks for 3 args start at arg 0,
it would now actually gets 3 args starting at arg 1.

> Where does it take `n' from?

From a quick audit:

sys_enter trace event, seccomp, & proc_pid_syscall gets n=6 args
starting at i=0.

Syscall tracing uses syscall metadata to fetch exactly the right number
of arguments starting at i=0 (this is where its particularly important
not to decrement n).

(This could probably be considered broken for the above cases of padded
64-bit args).

Also of interest is the kerneldoc comment describing this function in
include/asm-generic/syscall.h:

/**
 * syscall_get_arguments - extract system call parameter values
 * @task:	task of interest, must be blocked
 * @regs:	task_pt_regs() of @task
 * @i:		argument index [0,5]
 * @n:		number of arguments; n+i must be [1,6].
 * @args:	array filled with argument values
 *
 * Fetches @n arguments to the system call starting with the @i'th argument
 * (from 0 through 5).  Argument @i is stored in @args[0], and so on.
 * An arch inline version is probably optimal when @i and @n are constants.
 *
 * It's only valid to call this when @task is stopped for tracing on
 * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
 * It's invalid to call this with @i + @n > 6; we only support system calls
 * taking up to 6 arguments.
 */
void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
			   unsigned int i, unsigned int n, unsigned long *args);

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] MIPS: Fix buffer overflow in syscall_get_arguments()

[James Hogan]

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

On Mon, Feb 01, 2016 at 12:50:16PM +0000, Maciej W. Rozycki wrote:
> On Mon, 1 Feb 2016, James Hogan wrote:
> 
> > > > diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> > > > index 6499d93ae68d..47bc45a67e9b 100644
> > > > --- a/arch/mips/include/asm/syscall.h
> > > > +++ b/arch/mips/include/asm/syscall.h
> > > > @@ -101,10 +101,8 @@ static inline void syscall_get_arguments(struct task_struct *task,
> > > >  	/* O32 ABI syscall() - Either 64-bit with O32 or 32-bit */
> > > >  	if ((config_enabled(CONFIG_32BIT) ||
> > > >  	    test_tsk_thread_flag(task, TIF_32BIT_REGS)) &&
> > > > -	    (regs->regs[2] == __NR_syscall)) {
> > > > +	    (regs->regs[2] == __NR_syscall))
> > > >  		i++;
> > > > -		n++;
> > > > -	}
> > > >  
> > > >  	while (n--)
> > > >  		ret |= mips_get_syscall_arg(args++, task, regs, i++);
> > > 
> > >  What I think it really needs to do is to *decrease* the number of 
> > > arguments, as we're throwing the syscall number away as not an argument to 
> > > itself.  So this looks like a typo to me, the expression was meant to be 
> > > `n--' rather than `n++'.  With the number of arguments unchanged, as in 
> > > your proposed change, we're still reaching one word too far.
> > 
> > No, the caller asked for n args, thats what it should get. It doesn't
> > care whether the syscall was indirected or not.
> > 
> > The syscall doesn't have 1 less arg as a result of indirection. E.g.
> > What about system calls with 6 arguments, and hence 7 arguments
> > including syscall number argument when redirected? We'd get an
> > uninitialised 6th arg back when passing n=6.
> 
>  Do you mean the caller ignores the extra argument holding the number of 
> the wrapped syscall in counting syscall arguments?

No, the caller never sees the extra argument containing syscall number,
because i is incremented, so if caller asks for 3 args start at arg 0,
it would now actually gets 3 args starting at arg 1.

> Where does it take `n' from?

From a quick audit:

sys_enter trace event, seccomp, & proc_pid_syscall gets n=6 args
starting at i=0.

Syscall tracing uses syscall metadata to fetch exactly the right number
of arguments starting at i=0 (this is where its particularly important
not to decrement n).

(This could probably be considered broken for the above cases of padded
64-bit args).

Also of interest is the kerneldoc comment describing this function in
include/asm-generic/syscall.h:

/**
 * syscall_get_arguments - extract system call parameter values
 * @task:	task of interest, must be blocked
 * @regs:	task_pt_regs() of @task
 * @i:		argument index [0,5]
 * @n:		number of arguments; n+i must be [1,6].
 * @args:	array filled with argument values
 *
 * Fetches @n arguments to the system call starting with the @i'th argument
 * (from 0 through 5).  Argument @i is stored in @args[0], and so on.
 * An arch inline version is probably optimal when @i and @n are constants.
 *
 * It's only valid to call this when @task is stopped for tracing on
 * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
 * It's invalid to call this with @i + @n > 6; we only support system calls
 * taking up to 6 arguments.
 */
void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
			   unsigned int i, unsigned int n, unsigned long *args);

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]