linux-kernel - Re: [PATCH] futex: Robustify wake

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]

Date:	Wed, 18 Feb 2015 18:07:04 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Davidlohr Bueso <dave@...olabs.net>
Cc:	tglx@...utronix.de, peterz@...radead.org, dvhart@...ux.intel.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] futex: Robustify wake_futex()


* Davidlohr Bueso <dave@...olabs.net> wrote:

> Current code assumes that wake_futex() will never fail, 
> thus we are rather sloppy when incrementing the return 
> value in wake related calls, accounting for the newly 
> woken task. Of course this will never occur, thus not a 
> problem. This bug is as real as the need for the 
> redundant pi checks in wake_futex().
> 
> These redundant checks are fine and past discussion 
> indicates that they will stay. However, it does introduce 
> this mismatch, thus it is better to robustify the 
> function and avoid any assumptions that could bite us in 
> the arse the future.

So can the current code crash or hang if the WARN() 
triggers?

>  kernel/futex.c | 45 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 12 deletions(-)

My counter argument is that we add quite a bit of pointless 
complexity:

> -	if (WARN(q->pi_state || q->rt_waiter, "refusing to wake PI futex\n"))
> -		return;
> +	if (unlikely(WARN(q->pi_state || q->rt_waiter,
> +			  "refusing to wake PI futex\n")))
> +		return false;

> -			wake_futex(this);
> +			if (!wake_futex(this)) {
> +				ret = -EINVAL;
> +				break;
> +			}

  + [ 4 more usage sites ]

while the WARN() already told the user that the kernel is 
broken.

So what's the point? Does it avoid any real badness, state 
corruption, crash, hang, etc.?

Thanks,

	Ingo
--
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/