linux-kernel - Re: RFC: Fix f_flags races without the BKL

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:	Fri, 9 Jan 2009 11:08:21 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jonathan Corbet <corbet@....net>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andi Kleen <andi@...stfloor.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Al Viro <viro@...IV.linux.org.uk>, bfields@...ldses.org
Subject: Re: RFC: Fix f_flags races without the BKL

On 01/08, Jonathan Corbet wrote:
>
> This patch returns -ENOTTY in both places.  It seems better
> to me, but it *is* a change, and we may well not want to do that.

I'm afraid, this can break user-space applications. But I agree
it looks better.

>  static int setfl(int fd, struct file * filp, unsigned long arg)
>  {
> @@ -176,25 +179,52 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
>  	if (error)
>  		return error;
>
> -	/*
> -	 * We still need a lock here for now to keep multiple FASYNC calls
> -	 * from racing with each other.
> -	 */
> -	lock_kernel();
>  	if ((arg ^ filp->f_flags) & FASYNC) {
> -		if (filp->f_op && filp->f_op->fasync) {
> -			error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
> -			if (error < 0)
> -				goto out;
> -		}
> +		error = fasync_change(fd, filp, (arg & FASYNC) != 0);
> +		if (error < 0)
> +			goto out;

So, fasync_change() sets/clears FASYNC,

> +	lock_file_flags();
>  	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
> +	unlock_file_flags();

and then we change f_flags again, including F_ASYNC bit.

This is racy?

Suppose T1 does setfl(arg == 0) and preempted before lock_file_flags()
above. T2 does setfl(FASYNC) and succeeds. T1 resumes and clears FASYNC.
Now we have the same problem, the file's state is not consistent.

> +int fasync_change(int fd, struct file *filp, int on)
> +{
> +	int ret;
> +	static DEFINE_MUTEX(fasync_mutex);
> +
> +	if (filp->f_op->fasync == NULL)
> +		return -ENOTTY;
> +
> +	mutex_lock(&fasync_mutex);
> +	lock_file_flags();
> +	if (((filp->f_flags & FASYNC) == 0) == (on == 0)) {
> +		unlock_file_flags();
> +		return 0;
> +	}
> +	if (on)
> +		filp->f_flags |= FASYNC;
> +	else
> +		filp->f_flags &= ~FASYNC;
> +	unlock_file_flags();
> +	ret = filp->f_op->fasync(fd, filp, on);
> +	mutex_unlock(&fasync_mutex);
> +	return ret;

But we must not change ->f_flags if ->fasync() fails?

Now we have the global mutex for ->fasync... Well, not very
good but fasync_helper() takes fasync_lock anyway.

Oleg.

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