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

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

 



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: signature.asc
Description: Digital signature


[Index of Archives]     [Linux MIPS Home]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Linux]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux