linux-kernel - Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit handlers written in C

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:	Wed, 15 Jul 2015 12:56:35 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Kees Cook <keescook@...omium.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Denys Vlasenko <vda.linux@...glemail.com>,
	Ingo Molnar <mingo@...nel.org>,
	Brian Gerst <brgerst@...il.com>,
	Andy Lutomirski <luto@...capital.net>,
	Borislav Petkov <bp@...en8.de>,
	Andrew Lutomirski <luto@...nel.org>,
	Oleg Nesterov <oleg@...hat.com>, Peter Anvin <hpa@...or.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Rik van Riel <riel@...hat.com>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit
 handlers written in C

On Tue, Jul 14, 2015 at 4:07 PM, Frederic Weisbecker <fweisbec@...il.com> wrote:
> On Tue, Jul 07, 2015 at 03:51:48AM -0700, tip-bot for Andy Lutomirski wrote:
>> +     while (true) {
>> +             u32 cached_flags =
>> +                     READ_ONCE(pt_regs_to_thread_info(regs)->flags);
>> +
>> +             if (!(cached_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
>> +                                   _TIF_UPROBE | _TIF_NEED_RESCHED)))
>> +                     break;
>> +
>> +             /* We have work to do. */
>> +             local_irq_enable();
>> +
>> +             if (cached_flags & _TIF_NEED_RESCHED)
>> +                     schedule();
>> +
>> +             if (cached_flags & _TIF_UPROBE)
>> +                     uprobe_notify_resume(regs);
>> +
>> +             /* deal with pending signal delivery */
>> +             if (cached_flags & _TIF_SIGPENDING)
>> +                     do_signal(regs);
>> +
>> +             if (cached_flags & _TIF_NOTIFY_RESUME) {
>> +                     clear_thread_flag(TIF_NOTIFY_RESUME);
>> +                     tracehook_notify_resume(regs);
>> +             }
>> +
>> +             if (cached_flags & _TIF_USER_RETURN_NOTIFY)
>> +                     fire_user_return_notifiers();
>> +
>> +             /* Disable IRQs and retry */
>> +             local_irq_disable();
>> +     }
>
> I dreamed so many times about this loop in C!

So this made me look at it again, and now I'm worried.

There's that "early break", but it doesn't check
_TIF_USER_RETURN_NOTIFY. So if *only* USER_RETURN_NOTIFY is set, we're
screwed.

It migth be that that doesn't happen for some reason, but I'm not
seeing what that reason would be.

The other thing that worries me is that this depends on all the
handler routines to clear the flags (except for
tracehook_notify_resume()). Which they hopefully do. But that means
that just looking at this locally, it's not at all obvious that it
works right.

So wouldn't it be much nicer to do:

        u32 cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);

        cached_flags &= _TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
_TIF_USER_RETURN_NOTIFY | _TIF_UPROBE | _TIF_NEED_RESCHED;

        if (!cached_flags)
                break;

        atomic_clear_mask(cached_flags, &pt_regs_to_thread_info(regs)->flags);

and then have those bit tests after that?

                   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/