On Thu, Oct 16, 2014 at 1:42 AM, Peter Zijlstra <pet...@infradead.org> wrote: > On Tue, Oct 14, 2014 at 03:57:39PM -0700, Andy Lutomirski wrote: >> We currently allow any process to use rdpmc. This significantly >> weakens the protection offered by PR_TSC_DISABLED, and it could be >> helpful to users attempting to exploit timing attacks. >> >> Since we can't enable access to individual counters, use a very >> coarse heuristic to limit access to rdpmc: allow access only when >> a perf_event is mmapped. This protects seccomp sandboxes. >> >> There is plenty of room to further tighen these restrictions. For >> example, on x86, *all* perf_event mappings set cap_user_rdpmc. This >> should probably be changed to only apply to perf_events that are >> accessible using rdpmc. > > So I suppose this patch is a little over engineered,
:) I won't argue. > >> @@ -1852,10 +1865,26 @@ static ssize_t set_attr_rdpmc(struct device *cdev, >> if (x86_pmu.attr_rdpmc_broken) >> return -ENOTSUPP; >> >> + mutex_lock(&rdpmc_enable_mutex); >> if (!!val != !!x86_pmu.attr_rdpmc) { >> - x86_pmu.attr_rdpmc = !!val; >> - on_each_cpu(change_rdpmc, (void *)val, 1); >> + if (val) { >> + static_key_slow_inc(&rdpmc_enabled); >> + on_each_cpu(refresh_pce, NULL, 1); >> + smp_wmb(); >> + x86_pmu.attr_rdpmc = 1; >> + } else { >> + /* >> + * This direction can race against existing >> + * rdpmc-capable mappings. Try our best regardless. >> + */ >> + x86_pmu.attr_rdpmc = 0; >> + smp_wmb(); >> + static_key_slow_dec(&rdpmc_enabled); >> + WARN_ON(static_key_true(&rdpmc_enabled)); >> + on_each_cpu(refresh_pce, NULL, 1); >> + } >> } >> + mutex_unlock(&rdpmc_enable_mutex); >> >> return count; >> } > > why do you care about that rdpmc_enabled static key thing? Also you > should not expose static key control to userspace like this, they can > totally wreck the system. At the very least it should be > static_key_slow_dec_deferred() -- gawd I hate the static_key API. This particular control is only available to root, so I don't think it matters too much. I did it this way to avoid hitting an extra dcache line on every switch_mm. A nicer solution might be to track whether rdpmc is allowed for each perf_event and to count the number that allow rdpmc. That would cause 'echo 0 > rdpmc' to only work for new perf_events, but it fixes a race. Doing this will require passing more info to arch_perf_update_userpage, I think. Should I do that? It'll probably get a better result, but this patchset will get even longer and even more overengineered. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/