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: <20081203190648.GA23893@redhat.com>
Date:	Wed, 3 Dec 2008 20:06:48 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andi Kleen <ak@...ux.intel.com>
Cc:	Jonathan Corbet <corbet@....net>,
	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/02, Andi Kleen wrote:
>
>> 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.
>
> I wonder if we need FASYNC at all. This could be gotten implicitely by
> looking at the fasync_list

Only if socket.

Serioulsy, I think the best (partial, yes) fix for now is to restore
lock_kernel() in setfl() and change ioctl_fioxxx() accordingly.
At least this protect us from tty too.

I agree with Jonathan, we need the lock to protect ->f_flags, but
this needs changes. Personally, I do not like the global mutex,
perhaps we can use some "unused" bit in struct file. Say, ->f_mode
is never changed (afaics), we can place it here. Now, any code
which changes ->f_flags can do

	if (test_and_set_bit(FLAGS_LOCK_BIT))
		return;

	whatever();

	->f_flags = new_flags;

	clear_bit(FLAGS_LOCK_BIT);

No need to disable preemption, we never spin waiting for the
lock bit. If it is locked - somebody else updates ->f_flags,
we can pretend it does this after us. This can confuse F_GETFL
after F_SETFL (if F_SETFL "fails"), but I think in that case
user-space is wrong anyway, it must not do F_GETFL in parallel.

Not that I think this is very good idea though ;)

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/