linux-kernel - Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

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: <CA+55aFyOsPY-RNwedb3NnbKbj=iAc7L6YuSSEZYy5_Sczmqd4A@mail.gmail.com>
Date:	Thu, 10 May 2012 08:44:51 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jan Kiszka <jan.kiszka@...mens.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Michael Tokarev <mjt@....msk.ru>,
	Anthony Liguori <aliguori@...ibm.com>,
	Kevin Wolf <kwolf@...hat.com>
Subject: Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

On Thu, May 10, 2012 at 6:04 AM, Jan Kiszka <jan.kiszka@...mens.com> wrote:
> +               case SIG_BLOCK:
> +                       sigaddsetmask(&new_blocked, new_set);
> +                       break;
> +               case SIG_UNBLOCK:
> +                       sigdelsetmask(&new_blocked, new_set);
> +                       break;

Ok, I think SIG_[UN]BLOCK are now clearly right. However:

> +               case SIG_SETMASK:
> +                       new_blocked.sig[0] &=
> +                               ~((old_sigset_t)(compat_old_sigset_t)-1);
> +                       new_blocked.sig[0] |= new_set;
> +                       break;

I don't think this is clear.

The semantics for the *native* SIG_SETMASK has been to only change the
lower word of the sigset_t.

And that was actually defined in terms of "compat_sigset_word", not
"compat_old_sigset_t".

Now, they are both generally the same, and so I think your code does
the right thing, but I have to say that I really had to look closely
to make sure that yes, your code was right.

Anyway, my *gut* feel is that it would be much clearer to write the above as

    compat_sigset_word x = new_set;
    memcpy(new_blocked.sig, &x, sizeof(x));

together with a comment to the effect that sigprocmask(SIG_SETMASK..)
only changes the first word of the structure.

That said, I think your patch does look technically correct, so maybe
it's just me who thinks it is very non-obvious and hard to read.

The memcpy approach will also generate better code. This is the "mask-and-set":

        movabsq $-4294967296, %rax      #, tmp89
        andq    -32(%rbp), %rax # new_blocked.sig, tmp89
        orq     %rdx, %rax      # new_set, tmp89
        movq    %rax, -32(%rbp) # tmp89, new_blocked.sig

and this is the memcpy:

        movl    %edx, -32(%rbp) # new_set,

ie it is done as a simple 32-bit store.

I think I'll just edit your patch directly, no need to send me a new version.

                               Linus
--
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/