lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 29 Mar 2017 18:33:35 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Jan Kratochvil <jan.kratochvil@...hat.com>,
        Pedro Alves <palves@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: syscall_get_error() && TS_ checks

I must have missed something, but I simply can't undestand it.

	static inline long syscall_get_error(struct task_struct *task,
					     struct pt_regs *regs)
	{
		unsigned long error = regs->ax;
	#ifdef CONFIG_IA32_EMULATION
		/*
		 * TS_COMPAT is set for 32-bit syscall entries and then
		 * remains set until we return to user mode.
		 */
		if (task->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
			/*
			 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
			 * and will match correctly in comparisons.
			 */
			error = (long) (int) error;
	#endif
		return IS_ERR_VALUE(error) ? error : 0;
	}

Firstly, why do we need the IS_ERR_VALUE() check? This is only used by
do_signal/handle_signal, we do not care if it returns non-zero as long
as the value can't be confused with -ERESTART.* codes.

And why do we need the TS_ checks? IIUC only because a 32-bit debugger
can change regs->ax, and we also assume that if this happens outside of
syscall-exit path (so that TS_COMPAT is not set) then it should also
change regs->orig_ax and this implies TS_I386_REGS_POKED.

Oherwise it is not needed, even the 32-bit syscalls return long, not int.

So why we can't simply change putreg32() to always sign-extend regs->ax
regs->orig_ax and just do

	static inline long syscall_get_error(struct task_struct *task,
					     struct pt_regs *regs)
	{
		return regs-ax;
	}

? Or, better, simply kill it and use syscall_get_return_value() in
arch/x86/kernel/signal.c.

Of course, if the tracee is 64-bit and debugger is 32-bit then the
unconditional sign-extend can be wrong, but do we really care about
this case? This can't really work anyway. And the current code is not
right too. Say, debugger nacks the signal which interrupted syscall
and sets regs->ax = -ERESTARTSYS to force the restart, this won't work
because TS_COMPAT|TS_I386_REGS_POKED are not set.

Oleg.

Powered by blists - more mailing lists