linux-kernel - Re: [PATCH] Add a text

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, 27 Nov 2013 14:41:29 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	Andi Kleen <andi@...stfloor.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] Add a text_poke syscall v2

On Wed, Nov 27, 2013 at 2:02 PM, H. Peter Anvin <hpa@...or.com> wrote:
> For the record, this is the entire patch necessary to do the
> sync_cores() system call -- and no potential interactions with security
> frameworks or whatnot, simply because no security-sensitive operations
> are performed of any kind.
>
> Comments/opinions appreciated.

If we just care about a core sync, then:

 - on_each_cpu() is overkill, since you really only want each CPU that
can run that process.

   And we have that bitmask already: it's the same bitmask that we use
for TLB flush purposes.

 - calling "sync_cores()" cross-cpu is overkill, since the IPI will
already sync the other cores. And the current core is already going to
be synchronized wrt the code change, since we're in kernel mode and
the code is in user mode.

So I actually think your patch does too much - although it's possible
that we should have a system call argument saying "sync just this
process" or "sync all cores" so that people can literally do some
"modify global instruction in shared library" games if they really
really want to.

That said, the thing I really do *not* like about this patch is that
it makes the "insert 'int3' instruction" visible to user space. That
makes the "global instruction" replacement impossible, for example,
because while we'd sync all cores, we have no way to protect against
other processes getting the breakpoint exception and just SIGSEGV'ing
randomly as a result of *that*.

And even if we say "well, we don't care about the global case" (which
is quite possibly the right thign to do), it's actually hard for
threaded libraries to sanely catch SIGSGV for the breakpoint case too.
And since the only reason for this existing IN THE FIRST PLACE is the
threaded case, I have to say that I absolutely despise this "simpler"
interface.

The interface may be simpler, but it's garbage.

I didn't like the first patch either, but the first patch with
"replace the stupid pseudo-signal back-call with just waiting" at
least seems to be a good interface. Unlike this kind of stuff.

               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/