Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934336AbeAIQSC (ORCPT + 1 other); Tue, 9 Jan 2018 11:18:02 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:58679 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933583AbeAIQR6 (ORCPT ); Tue, 9 Jan 2018 11:17:58 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Dan Williams Cc: Linux Kernel Mailing List , linux-arch@vger.kernel.org, Peter Zijlstra , Netdev , Greg KH , Thomas Gleixner , Linus Torvalds , "David S. Miller" , Elena Reshetova , Alan Cox References: <151520099201.32271.4677179499894422956.stgit@dwillia2-desk3.amr.corp.intel.com> <151520108080.32271.16420298348259030860.stgit@dwillia2-desk3.amr.corp.intel.com> <87lgh7n2tf.fsf@xmission.com> Date: Tue, 09 Jan 2018 10:17:08 -0600 In-Reply-To: (Dan Williams's message of "Mon, 8 Jan 2018 19:42:14 -0800") Message-ID: <87373fj9bv.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1eYwb2-0007lA-A6;;;mid=<87373fj9bv.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.133.177;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+AT2yeRJlhxqd55P5SvaTY1FR9p+KqxXA= X-SA-Exim-Connect-IP: 67.3.133.177 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 16/18] net: mpls: prevent bounds-check bypass via speculative execution X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Dan Williams writes: > On Mon, Jan 8, 2018 at 7:11 PM, Eric W. Biederman wrote: >> Dan Williams writes: >> >>> Static analysis reports that 'index' may be a user controlled value that >>> is used as a data dependency reading 'rt' from the 'platform_label' >>> array. In order to avoid potential leaks of kernel memory values, block >>> speculative execution of the instruction stream that could issue further >>> reads based on an invalid 'rt' value. >> >> >> In detail. >> a) This code is fast path packet forwarding code. Introducing an >> unconditional pipeline stall is not ok. >> >> AKA either there is no speculation and so this is invulnerable >> or there is speculation and you are creating an unconditional >> pipeline stall here. >> >> My back of the napkin caluculations say that a pipeline stall >> is about 20 cycles. Which is about the same length of time >> as a modern cache miss. >> >> On a good day this code will perform with 0 cache misses. On a less >> good day 1 cache miss. Which means you are quite possibly doubling >> the runtime of mpls_forward. >> >> b) The array is dynamically allocated which should provide some >> protection, as it will be more difficult to predict the address >> of the array which is needed to craft an malicious userspace value. >> >> c) The code can be trivially modified to say: >> >> static struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index) >> { >> struct mpls_route *rt = NULL; >> >> if (index < net->mpls.platform_labels) { >> struct mpls_route __rcu **platform_label = >> rcu_dereference(net->mpls.platform_label); >> rt = rcu_dereference(platform_label[index & ((1 << 20) - 1)]); >> } >> return rt; >> } >> >> AKA a static mask will ensure that there is not a primitive that can be >> used to access all of memory. That is max a 1 cycle slowdown in the >> code, which is a much better trade off. >> >> d) If we care more it is straight forward to modify >> resize_platform_label_table() to ensure that the size of the array >> is always a power of 2. >> >> e) The fact that a pointer is returned from the array and it is treated >> like a pointer would seem to provide a defense against the >> exfiltration technique of using the value read as an index into >> a small array, that user space code can probe aliased cached >> lines of, to see which value was dereferenced. >> >> >> So to this patch in particular. >> Nacked-by: "Eric W. Biederman" >> >> This code path will be difficult to exploit. This change messes with >> performance. There are ways to make this code path useless while >> preserving the performance of the code. >> > > Thanks, Eric understood. The discussion over the weekend came to the > conclusion that using a mask will be the default approach. The > nospec_array_ptr() will be defined to something similar to the > following, originally from Linus and tweaked by Alexei and I: > > #define __nospec_array_ptr(base, idx, sz) \ > ({ \ > union { typeof(&base[0]) _ptr; unsigned long _bit; } __u; \ > unsigned long _i = (idx); \ > unsigned long _m = (max); \ > unsigned long _mask = ~(long)(_m - 1 - _i) >> BITS_PER_LONG - 1;\ > OPTIMIZER_HIDE_VAR(_mask); \ > __u._ptr = &base[_i & _mask]; \ > __u._bit &= _mask; \ > __u._ptr; \ > }) > > Does that address your performance concerns? The user controlled value condition of your patchset implies that you assume indirect branch predictor poisoning is handled in other ways. Which means that we can assume speculation will take some variation of the static call chain. Further you are worrying about array accesses. Which means you are worried about attacks that are the equivalent of meltdown that can give you reading of all memory available to the kernel. The mpls code in question reads a pointer from memory. The only thing the code does with that pointer is verify it is not NULL and dereference it. That does not make it possible to extricate the pointer bits via a cache side-channel as a pointer is 64bits wide. There might maybe be a timing attack where it is timed how long the packet takes to deliver. If you can find the base address of the array, at best such a timeing attack will tell you is if some arbitrary cache line is already cached in the kernel. Which is not the class of attack your patchset is worried about. Further there are more direct ways to probe the cache from a local process. So I submit to you that the mpls code is not vulnerable to the class of attack you are addressing. Further I would argue that anything that reads a pointer from memory is a very strong clue that it falls outside the class of code that you are addressing. Show me where I am wrong and I will consider patches. Eric