linux-kernel - Re: Q: sys_futex() && timespec

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1006252140410.18683@localhost.localdomain>
Date:	Fri, 25 Jun 2010 21:59:09 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Oleg Nesterov <oleg@...hat.com>
cc:	Darren Hart <dvhltc@...ibm.com>, Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Andreas Schwab <schwab@...hat.com>,
	Danny Feng <dfeng@...hat.com>,
	Jakub Jelinek <jakub@...hat.com>,
	Ulrich Drepper <drepper@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: sys_futex() && timespec_valid()

B1;2005;0cOleg,

On Fri, 25 Jun 2010, Oleg Nesterov wrote:

> Hello.
> 
> Another stupid question about the trivial problem I am going to ask,
> just to report the authoritative answer back to bugzilla. The problem
> is, personally I am not sure we should/can add the user-visible change
> required by glibc maintainers, and I am in no position to suggest them
> to fix the user-space code instead.
> 
> 
> In short, glibc developers believe that sys_futex(ts) is buggy and
> needs the fix to return -ETIMEDOUT instead of -EINVAL in case when
> ts->tv_sec < 0 and the timeout is absolute.

Oh well. We followed the validity check for all other syscalls which
hand in [absolute] timespecs:

 The rqtp argument specified a nanosecond value less than zero or
 greater than or equal to 1000 million; or the TIMER_ABSTIME flag was
 specified in flags and the rqtp argument is outside the range for the
 clock specified by clock_id;

tv->sec < 0 is definitely an invalid value for both CLOCK_REALTIME and
CLOCK_MONOTONIC. And I consider any code assuming that it's sane as
buggy by definition.

I'm strictly against having different definitions of sanity for
different syscalls.

> Ignoring the possible cleanups/microoptimizations, something like this:
> 
> --- x/kernel/futex.c
> +++ x/kernel/futex.c
> @@ -2625,6 +2625,16 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
>  		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
>  		if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
>  			return -EFAULT;
> +
> +		// absolute timeout
> +		if (cmd != FUTEX_WAIT) {
> +			if (ts->tv_nsec >= NSEC_PER_SEC)
> +				return -EINVAL;
> +			if (ts->tv_sec < 0)
> +				return -ETIMEDOUT;
> +		}
> +
> +
>  		if (!timespec_valid(&ts))
>  			return -EINVAL;

 Btw, you'd need that ugly check in the compat syscall as well.
 
> ------------------------------------------------------------------------
> 
> Otherwise, pthread_rwlock_timedwrlock(ts) hangs spinning in user-space
> forever if ts->tv_sec < 0.
> 
> To clarify: this depends on libc version and arch.

Ouch. So we have code in libc which makes different assumptions about
the syscall semantics ?

> This happens because pthread_rwlock_timedwrlock(rwlock, ts) on x86_64
> roughly does:
> 
> 	for (;;) {
> 		if (fast_path_succeeds(rwlock))
> 			return 0;
> 
> 		if (ts->tv_nsec >= NSEC_PER_SEC)
> 			return EINVAL;
> 
> 		errcode = sys_futex(FUTEX_WAIT_BITSET_PRIVATE, ts);
> 		if (errcode == ETIMEDOUT)
> 			return ETIMEDOUT;
> 	}
> 
> and since the kernel return EINVAL due to !timespec_valid(ts), the
> code above loops forever.
> 
> (btw, we have same problem with EFAULT, and this is considered as
>  a caller's problem).

Brilliant.
 
> IOW, pthread_rwlock_timedwrlock() assumes that in this case
> sys_futex() can return nothing interesting except 0 or ETIMEDOUT.
> I guess pthread_rwlock_timedwrlock() is not alone, but I didn't check.
> 
> So, the question: do you think we can change sys_futex() to make
> glibc happy?

Do we really want to add crap to the kernel, just because some
lunatics have interesting assumptions about validation ?

Definitely NOT
 
> Or, do you think it is user-space who should check tv_sec < 0 if
> it wants ETIMEDOUT with the negative timeout ?

If user space folks consider tv_sec < 0 a value which is sane and
inside the valid range of CLOCK_MONO/REAL then I can't do much more
than shrug.

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/