Removing per-task TSD? (Re: Tightening up rdpmc permissions?)

Removing per-task TSD? (Re: Tightening up rdpmc permissions?)

[Andy Lutomirski]

On Mon, Sep 29, 2014 at 11:42 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sep 29, 2014 10:36 AM, <Valdis.Kletnieks@vt.edu> wrote:
>>
>> On Mon, 29 Sep 2014 09:39:16 -0700, Andy Lutomirski said:
>>
>> > Would it make sense to restrict rdpmc to tasks that are in mms that
>> > have a perf_event mapping?  After all, unless I misunderstand
>> > something, user code can't reliably use rdpmc unless they've mapped a
>> > perf_event object to check the rdpmc bit and figure out what ecx value
>> > to use.
>>
>> Wouldn't that be trivially easy for an attacker to bypass? Just map a dummy
>> perf_event object  and then go to town?
>
> Depends on the paranoia setting.  We could require that the mapped
> object actually have an rdpmc-able counter running.
>
> Seccomp can (and often does) block access to perf_event_open entirely.
> We could certainly change the code to only twiddle CR4 if TIF_SECCOMP
> or TIF_NOTSC is set.  I think that the real thing we should optimize
> for is to minimize the chance that a given context switch actually
> needs to *change* cr4.  Since perf_event maps are relatively unusual,
> at least only perf-using programs would have overhead if we just gated
> it on the existence of a useful rdpmc-able mapping.

So this is a mess.  I think that any reasonable implementation of
rdpmc permissions should be per mm, since we perf_event maps are, of
course, per mm.

Similarly, any reasonable implementation of rdtsc permissions should
be per mm, since doing it sensibly involves telling the vdso not to
use rdtsc, and the vdso is per mm

Unfortunately, PR_SET_TSC is a per-thread setting.  Implementing this
correctly looks like it'll require twiddling, or at least thinking
about, cr4 at switch_mm time *and* when switching tasks, because the
only sensible way of granting PMC access to a running mm is to
broadcast a function call to the cpus running that mm.

Nonetheless, this is doable.  Either there can be separate context
switching of CR4.PCE (in switch_mm) and CR4.TSD (in switch_to), or
there can be some crazy optimization to make it faster.

All of this sucks, so I'll ask a normally verboten question: can we
just remove PR_SET_TSC entirely?

No, really.  It's been effectively broken as a security measure for a
*long* time, since there's no protection of rdpmc, and rdpmc can *read
the cycle counter*.  It's also mostly broken because any process,
without any exception whatsoever (until 3.16 [1]) or with technical
exceptions that I virtually guarantee that no one uses (since I wrote
the code, and it's very recent!) can read the vvar timing data or the
fscking HPET (!).

The correct (IMO) long-term solution is to gate both rdpmc and rdtsc
on a per-mm basis and to teach the vdso to cope.  There's demand for
the latter feature from the CRIU camp, as well.

Thoughts?  Can we remove a security feature that hasn't provided any
security for years to make room for a new security feature that
doesn't suck?

[1] Even 32-bit processes can.  All they have to do is to long jump to
64 bit mode and read a fixed address.  Piece of cake!

--Andy

>
> (Also, why on earth is TIF_NOTSC a thread_info flag?  Wouldn't just
> adding a field "cr4" to task_struct or something be simpler and quite
> possibly faster?  We have a branch anyway...)
>
> --Andy



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: Removing per-task TSD? (Re: Tightening up rdpmc permissions?)

[Peter Zijlstra]

On Fri, Oct 03, 2014 at 09:41:30AM -0700, Andy Lutomirski wrote:
> So this is a mess.  I think that any reasonable implementation of
> rdpmc permissions should be per mm, since we perf_event maps are, of
> course, per mm.
> 
> Similarly, any reasonable implementation of rdtsc permissions should
> be per mm, since doing it sensibly involves telling the vdso not to
> use rdtsc, and the vdso is per mm

So far so good.

> Unfortunately, PR_SET_TSC is a per-thread setting.  Implementing this
> correctly looks like it'll require twiddling, or at least thinking
> about, cr4 at switch_mm time *and* when switching tasks, because the
> only sensible way of granting PMC access to a running mm is to
> broadcast a function call to the cpus running that mm.

Confused... now though.

Any cpu can only ever run one mm at the time, and the only way to change
mm on any one cpu is switch_mm(), so having a CR4 write in switch_mm()
will DTRT, no weird broadcasts or anything else required.

Or were you talking about doing the per mm filter while maintaining the
old TIF_NOTSC thing? Then still no broadcasts would be required I think.

> Nonetheless, this is doable.  Either there can be separate context
> switching of CR4.PCE (in switch_mm) and CR4.TSD (in switch_to), or
> there can be some crazy optimization to make it faster.

IFF you want to retain TIF_NOTSC then yes, we'll need both or crazy.

> All of this sucks, so I'll ask a normally verboten question: can we
> just remove PR_SET_TSC entirely?

Fine with me, see more in that thread, I'll try and reply to the right
copy.

Re: Removing per-task TSD? (Re: Tightening up rdpmc permissions?)

[Andy Lutomirski]

On Fri, Oct 3, 2014 at 1:06 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Oct 03, 2014 at 09:41:30AM -0700, Andy Lutomirski wrote:
>> So this is a mess.  I think that any reasonable implementation of
>> rdpmc permissions should be per mm, since we perf_event maps are, of
>> course, per mm.
>>
>> Similarly, any reasonable implementation of rdtsc permissions should
>> be per mm, since doing it sensibly involves telling the vdso not to
>> use rdtsc, and the vdso is per mm
>
> So far so good.
>
>> Unfortunately, PR_SET_TSC is a per-thread setting.  Implementing this
>> correctly looks like it'll require twiddling, or at least thinking
>> about, cr4 at switch_mm time *and* when switching tasks, because the
>> only sensible way of granting PMC access to a running mm is to
>> broadcast a function call to the cpus running that mm.
>
> Confused... now though.
>
> Any cpu can only ever run one mm at the time, and the only way to change
> mm on any one cpu is switch_mm(), so having a CR4 write in switch_mm()
> will DTRT, no weird broadcasts or anything else required.

I have it working, with no change to TIF_NOTSC.  I'm not totally
thrilled by the approach, but I think it's okay, it should have no
effect on any visible ABI, and it's decently fast.

>
> Or were you talking about doing the per mm filter while maintaining the
> old TIF_NOTSC thing? Then still no broadcasts would be required I think.
>
>> Nonetheless, this is doable.  Either there can be separate context
>> switching of CR4.PCE (in switch_mm) and CR4.TSD (in switch_to), or
>> there can be some crazy optimization to make it faster.
>
> IFF you want to retain TIF_NOTSC then yes, we'll need both or crazy.

I actually think that crazy isn't so bad.  switch_mm knows the task
it's switching to, so it can pre-load cr4.  My patch works by creating
a per-cpu cr4 shadow, so pre-loading cr4 for the next task a little
bit early ought to work without any expensive microcoded operations.

I'll send patches reasonably soon.

--Andy

>
>> All of this sucks, so I'll ask a normally verboten question: can we
>> just remove PR_SET_TSC entirely?
>
> Fine with me, see more in that thread, I'll try and reply to the right
> copy.



-- 
Andy Lutomirski
AMA Capital Management, LLC