linux-kernel - Re: BUG? "Call fasync() functions without the BKL" is racy

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: <20081202122939.GA3619@redhat.com>
Date:	Tue, 2 Dec 2008 13:29:39 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jonathan Corbet <corbet@....net>
Cc:	Andi Kleen <ak@...ux.intel.com>, Al Viro <viro@...iv.linux.org.uk>,
	Vitaly Mayatskikh <vmayatsk@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: BUG? "Call fasync() functions without the BKL" is racy

On 12/01, Jonathan Corbet wrote:
>
> On Mon, 1 Dec 2008 20:15:55 +0100
> Oleg Nesterov <oleg@...hat.com> wrote:
>
> > > Hmm, about checking for this case and retrying?
> > >
> > > Or put a fasync mutex into files_struct.  
> >
> > Perhaps, we can add O_LOCK_FLAGS, then something like
> >
> > 	--- a/fs/fcntl.c
> > 	+++ b/fs/fcntl.c
> > 	@@ -175,6 +175,15 @@ static int setfl(int fd, struct file * f
> > 		if (error)
> > 			return error;
> >
> > 	+	spin_lock(&current->files->file_lock);
> > 	+	if (!(filp->f_flags & O_LOCK_FLAGS))
> > 	+		filp->f_flags |= O_LOCK_FLAGS;
> > 	+	else
> > 	+		error = -EAGAIN;
> > 	+	spin_unlock(&current->files->file_lock);
> > 	+	if (error) /* pretend ->f_flags was changed after us
> > */
> > 	+		return 0;
> > 	+
>
> This strikes me as overkill.

and doesn't really help. fork() clones files_struct, we can't rely
on ->file_lock. And we have fd passing.

> What we really want to do is to protect
> against concurrent access to f_flags - something which could come about
> in a couple of other situations (nfsd/vfs.c tweaks it, for example).
> We *could* just extend files_lock to cover f_flags too, but that comes
> at the cost of making ->fasync() atomic when it never has been before -
> doesn't seem like a good idea.
>
> Perhaps we just need a single f_flags_mutex?  For code changing
> f_flags only (it woudn't be needed to query the flags)? Then
> ioctl_fionbio() and ioctl_fioasync() could use it too. It's hard to
> imagine that there's enough contention to warrant any more than that,
> especially given that it all (except ioctl_fionbio()) has been under
> the BKL until now.

A bit ugly to use the global mutex to protect all ->f_flags, but yes,
I agree. Imho this is 2.6.28 material, we need the simple fix or the
user can crash the kernel.

Still I'd like to see the better fix for the long term, the only (afaics)
flag with the "side effect" is FASYNC, perhaps we can move it to (say)
->f_mode, but this is ugly of course and still need serialization for the
pathes which play with FASYNC.

Otherwise, any code which changes ->f_flags can clear FASYNC if we race
with sys_fcntl(F_SETFL, FASYNC).

If we introduce the new lock, we should change for example tty_io.c:fionbio(),
tty has the ->fasync() method. Currently tty_io.c relies on lock_kernel().

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/