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]

Date:   Sun, 19 Dec 2021 21:25:44 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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 12/19/21 12:14 PM, Linus Torvalds wrote:
...
> 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.

The modification is definitely thread-local, but the implications are
wider after the dynamic xfeature and sigframe support went in.  Now,
(x86-only) no thread is allowed to enable dynamic features unless the
entire _process's_ altstacks pass validate_sigaltstack().

> 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".

The patch definitely makes the code easier to read.  But, it looks like
we need to invert the sigaltstack_size_valid() condition from the patch:

> +               if (unlikely(ss_size < min_ss_size) ||
> +                   unlikely(sigaltstack_size_valid(ss_size))) {
			       ^^^^^
> +                       sigaltstack_unlock();
> +                       return -ENOMEM;
>                 }

That should be !sigaltstack_size_valid(ss_size).

Also, the sigaltstack_lock() lock really is needed over the assignments
like this:

>                 t->sas_ss_sp = (unsigned long) ss_sp;
>                 t->sas_ss_size = ss_size;
>                 t->sas_ss_flags = ss_flags;
to prevent races with validate_sigaltstack().  We desperately need a
comment in there, but we probably shouldn't reference
validate_sigaltstack() itself since it's deeply x86-only.  I've got a
shot at a comment in the attached patch.

As for the the:

>                 if (ss_mode == SS_DISABLE) {
>                         t->sas_ss_sp = 0;
>                         t->sas_ss_size = 0;
>                         t->sas_ss_flags = ss_flags;
>                         return 0;
>                 }

hunk, I think it's OK.  Shrinking t->sas_ss_size without the lock is
safe-ish because it will never cause validate_sigaltstack() to fail.  I
need to think about that bit more, though.

Another blatantly untested patch is attached.  I'll try to give it a go
on some real hardware tomorrow.

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