lkml.org 
[lkml]   [2021]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [GIT PULL] core/urgent for v5.16-rc6
    From
    Date
    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.

    1 file changed, 19 insertions(+), 17 deletions(-)

    index dfcee3888b00..f58f1d574931 100644

    ---

    b/kernel/signal.c | 41 ++++++++++++++++++++++++-----------------
    1 file changed, 24 insertions(+), 17 deletions(-)

    diff -puN kernel/signal.c~linux-sigaltstack kernel/signal.c
    --- a/kernel/signal.c~linux-sigaltstack 2021-12-19 16:50:41.411762535 -0800
    +++ b/kernel/signal.c 2021-12-19 21:14:14.605399136 -0800
    @@ -4161,7 +4161,6 @@ do_sigaltstack (const stack_t *ss, stack
    size_t min_ss_size)
    {
    struct task_struct *t = current;
    - int ret = 0;

    if (oss) {
    memset(oss, 0, sizeof(stack_t));
    @@ -4181,8 +4180,15 @@ do_sigaltstack (const stack_t *ss, stack
    return -EPERM;

    ss_mode = ss_flags & ~SS_FLAG_BITS;
    - if (unlikely(ss_mode != SS_DISABLE && ss_mode != SS_ONSTACK &&
    - ss_mode != 0))
    +
    + if (ss_mode == SS_DISABLE) {
    + t->sas_ss_sp = 0;
    + t->sas_ss_size = 0;
    + t->sas_ss_flags = ss_flags;
    + return 0;
    + }
    +
    + if (unlikely(ss_mode != SS_ONSTACK && ss_mode != 0))
    return -EINVAL;

    /*
    @@ -4194,24 +4200,25 @@ do_sigaltstack (const stack_t *ss, stack
    t->sas_ss_flags == ss_flags)
    return 0;

    + /*
    + * Lock out any changes to sigaltstack_size_valid()
    + * until the t->sas_ss_* changes are complete:
    + */
    sigaltstack_lock();
    - if (ss_mode == SS_DISABLE) {
    - ss_size = 0;
    - ss_sp = NULL;
    - } else {
    - if (unlikely(ss_size < min_ss_size))
    - ret = -ENOMEM;
    - if (!sigaltstack_size_valid(ss_size))
    - ret = -ENOMEM;
    - }
    - if (!ret) {
    - t->sas_ss_sp = (unsigned long) ss_sp;
    - t->sas_ss_size = ss_size;
    - t->sas_ss_flags = ss_flags;
    +
    + if (unlikely(ss_size < min_ss_size) ||
    + unlikely(!sigaltstack_size_valid(ss_size))) {
    + sigaltstack_unlock();
    + return -ENOMEM;
    }
    +
    + t->sas_ss_sp = (unsigned long) ss_sp;
    + t->sas_ss_size = ss_size;
    + t->sas_ss_flags = ss_flags;
    +
    sigaltstack_unlock();
    }
    - return ret;
    + return 0;
    }

    SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss)
    _
    \
     
     \ /
      Last update: 2021-12-20 06:26    [W:5.114 / U:1.612 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site