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

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

 



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


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