[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <151571808858.27429.12915543823772169761.stgit@dwillia2-desk3.amr.corp.intel.com>
Date: Thu, 11 Jan 2018 16:48:08 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: linux-kernel@...r.kernel.org
Cc: linux-arch@...r.kernel.org, akpm@...ux-foundation.org,
kernel-hardening@...ts.openwall.com, netdev@...r.kernel.org,
"Eric W. Biederman" <ebiederm@...ssion.com>, tglx@...utronix.de,
torvalds@...ux-foundation.org, "David S. Miller" <davem@...emloft.net>,
Elena Reshetova <elena.reshetova@...el.com>, alan@...ux.intel.com
Subject: [PATCH v2 19/19] net: mpls: prevent bounds-check bypass via
speculative execution
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.
Based on an original patch by Elena Reshetova.
Eric notes:
"
When val is a pointer not an integer.
Then
array2[val] = y;
/* or */
y = array2[va];
Won't happen.
val->field;
Will happen.
Which looks similar. However the address space of pointers is too
large. Making it impossible for an attack to know where to look in
the cache to see if "val->field" happened. At least on the
assumption that val is an arbitrary value.
Further mpls_forward is small enough the entire scope of "rt" the
value read possibly past the bound check is auditable without too
much trouble. I have looked and I don't see anything that could
possibly allow the value of "rt" to be exfitrated. The problem
continuing to be that it is a pointer and the only operation on the
pointer besides derferencing it is testing if it is NULL.
Other types of timing attacks are very hard if not impossible
because any packet presenting with a value outside the bounds check
will be dropped. So it will hard if not impossible to find
something to time to see how long it took to drop the packet.
"
The motivation of resending this patch despite the NAK is to
continue a community wide discussion on the bar for judging Spectre
changes. I.e. is any user controlled speculative pointer in the
kernel a pointer too far, especially given the current array_ptr()
implementation.
Cc: "David S. Miller" <davem@...emloft.net>
Cc: Eric W. Biederman <ebiederm@...ssion.com>
Cc: netdev@...r.kernel.org
Signed-off-by: Elena Reshetova <elena.reshetova@...el.com>
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
net/mpls/af_mpls.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 8ca9915befc8..c92b1033adc2 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -8,6 +8,7 @@
#include <linux/ipv6.h>
#include <linux/mpls.h>
#include <linux/netconf.h>
+#include <linux/nospec.h>
#include <linux/vmalloc.h>
#include <linux/percpu.h>
#include <net/ip.h>
@@ -77,12 +78,13 @@ static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt,
static struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index)
{
struct mpls_route *rt = NULL;
+ struct mpls_route __rcu **platform_label =
+ rcu_dereference(net->mpls.platform_label);
+ struct mpls_route __rcu **rtp;
- if (index < net->mpls.platform_labels) {
- struct mpls_route __rcu **platform_label =
- rcu_dereference(net->mpls.platform_label);
- rt = rcu_dereference(platform_label[index]);
- }
+ rtp = array_ptr(platform_label, index, net->mpls.platform_labels);
+ if (rtp)
+ rt = rcu_dereference(*rtp);
return rt;
}
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
