linux-kernel - Re: [RFC][PATCH] seccomp: Fail immediately if any thread is performing an exec

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] [day] [month] [year] [list]

Date:   Tue, 18 Aug 2020 10:51:33 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>
Subject: Re: [RFC][PATCH] seccomp: Fail immediately if any thread is
 performing an exec

On Mon, Aug 17, 2020 at 12:11 PM Eric W. Biederman
<ebiederm@...ssion.com> wrote:
>
> +static void set_in_execve(bool in_execve)
> +{
> +       struct task_struct *me = current;
> +       spinlock_t *lock = &me->sighand->siglock;
> +
> +       spin_lock_irq(lock);
> +       me->in_execve = in_execve;
> +       spin_unlock_irq(lock);
> +}

No. This is complete voodoo programming. Code like this makes
absolutely zero sense.

Using a lock to serialize a single write is completely bogus. Yes, it
can be required if the field is a bitfield and you want to protect the
other bits in the word, but then you shouldn't be using a bitfield.

It adds zero serialization that a WRITE_ONCE/READ_ONCE pair doesn't
add. The other side will get either the old or the new value
regardless of the locking, so the locking is completely worthless
garbage.

At best it's just wasted CPU time. At worst, it confuses people about
what the locking means and results in bugs down the line. Don't do
things like this.

Locking for reading or writing a single value makes no sense. Locking
is only useful when there is a coherency issue and you have a
*sequence* of writes.

If you have a single value that acts as a flag, use
READ_ONCE/WRITE_ONCE to show that there's no relevant locking. In
fact, better yet, use "smp_store_release()" to set the flag and
"smp_load_acquire()" to read it, and then you get the read/write once
semantics _and_ an ordering between the "I have started doing X,
everything I've done up until this point is now guaranteed to be
visible to whoever reads this value".

           Linus

PS. Yes, there are situations where you want to lock around a single
write because of the serialization it guarantees for things around the
locked code. So you can have valid "lock-write-unlock" sequences. But
then you'd better have a BIG HONKING COMMENT about what the hell
you're really serializing, because it's not the single write, it's
something bigger.