linux-kernel - Re: [GIT PULL] core/urgent for v5.16-rc6

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: <CAHk-=wiNMghi=nZc432_Sj4QwG+BtxGUtovnpVQk-LpDj8r3ZA@mail.gmail.com>
Date:   Sun, 19 Dec 2021 12:14:29 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Borislav Petkov <bp@...e.de>,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] core/urgent for v5.16-rc6

On Sun, Dec 19, 2021 at 5:40 AM Borislav Petkov <bp@...e.de> wrote:
>
> Prevent lock contention on the new sigaltstack lock on the common-case
> path, when no changes have been made to the alternative signal stack.

I pulled this, but I think it's wrong.

It checks whether the new ss_size/ss_sp is the same as the old ones,
and skips the work if so.

But that check is bogus.

Why? Because it's comparing the wrong values.

It's comparing the values as they are *before* they are fixed up for
the SS_DISABLE case.

So if the new mode is SS_DISABLE, it's going to compare the values
against the old values for ss_size and ss_sp: but those old values
will have been reset to 0/NULL.

And the new values have not been reset yet before the comparison.

So the comparison wil easily fail when it shouldn't.

And that's all pointless anyway. If it's SS_DISABLE, there's no point
in doing *any* of this.

Now, I decided to keep the pull, because this bug only means that the
commit isn't actually as effective as it *should* be.

Honestly, that do_sigaltstack code is written just about pessimally,
and taking the sigaltstack_lock just for the limit checking is all
kinds of silly.

The SS_DISABLE case shouldn't take the lock at all.

And the actual modification of the values shouldn't need any locking
at all, since it's all thread-local.

I'm not convinced even the limit checking needs the lock, but
whatever. I think it could maybe just use "read_once()" or something.

I think the attached patch is an improvement, but I did *not* test
this, and I'm just throwing this out as a "maybe something like this".

Comments?

Note: I will throw this patch away after sending it out. If people agree,

                  Linus

View attachment "patch.diff" of type "text/x-patch" (1830 bytes)