[PATCH v4.1 10/10] nl80211: sanitize array index in parse_txq_params
Wireless drivers rely on parse_txq_params to validate that
txq_params->ac is less than NL80211_NUM_ACS by the time the low-level
driver's ->conf_tx() handler is called. Use a new helper, 'array_idx',
to sanitize txq_params->ac with respect to speculation. I.e. ensure that
any speculation into ->conf_tx() handlers is done with a value of
txq_params->ac that is within the bounds of [0, NL80211_NUM_ACS).
Reported-by: Christian Lamparter
Reported-by: Elena Reshetova
Cc: Johannes Berg
Cc: "David S. Miller"
Cc: linux-wireless@vger.kernel.org
Signed-off-by: Dan Williams
---
include/linux/nospec.h | 21 +
net/wireless/nl80211.c | 10 +++---
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index dd3aa05fab87..b8a9222e34d1 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -41,4 +41,25 @@ static inline unsigned long array_ptr_mask(unsigned long
idx, unsigned long sz)
__u._bit &= _mask; \
__u._ptr; \
})
+
+/**
+ * array_idx - Generate a pointer to an array index, ensuring the
+ * pointer is bounded under speculation to NULL.
+ *
+ * @idx: the index of the element, must be less than LONG_MAX
+ * @sz: the number of elements in the array, must be less than LONG_MAX
+ *
+ * If @idx falls in the interval [0, @sz), returns &@idx otherwise
+ * returns NULL.
+ */
+#define array_idx(idx, sz) \
+({ \
+ union { typeof((idx)) *_ptr; unsigned long _bit; } __u; \
+ typeof(idx) *_i = &(idx); \
+ unsigned long _mask = array_ptr_mask(*_i, (sz));\
+ \
+ __u._ptr = _i; \
+ __u._bit &= _mask; \
+ __u._ptr; \
+})
#endif /* __NOSPEC_H__ */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d396cb61a280..e3b3527c16d4 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -2056,20 +2057,23 @@ static const struct nla_policy
txq_params_policy[NL80211_TXQ_ATTR_MAX + 1] = {
static int parse_txq_params(struct nlattr *tb[],
struct ieee80211_txq_params *txq_params)
{
+ u8 ac, *idx;
+
if (!tb[NL80211_TXQ_ATTR_AC] || !tb[NL80211_TXQ_ATTR_TXOP] ||
!tb[NL80211_TXQ_ATTR_CWMIN] || !tb[NL80211_TXQ_ATTR_CWMAX] ||
!tb[NL80211_TXQ_ATTR_AIFS])
return -EINVAL;
- txq_params->ac = nla_get_u8(tb[NL80211_TXQ_ATTR_AC]);
+ ac = nla_get_u8(tb[NL80211_TXQ_ATTR_AC]);
txq_params->txop = nla_get_u16(tb[NL80211_TXQ_ATTR_TXOP]);
txq_params->cwmin = nla_get_u16(tb[NL80211_TXQ_ATTR_CWMIN]);
txq_params->cwmax = nla_get_u16(tb[NL80211_TXQ_ATTR_CWMAX]);
txq_params->aifs = nla_get_u8(tb[NL80211_TXQ_ATTR_AIFS]);
- if (txq_params->ac >= NL80211_NUM_ACS)
+ idx = array_idx(ac, NL80211_NUM_ACS);
+ if (!idx)
return -EINVAL;
-
+ txq_params->ac = *idx;
return 0;
}
[PATCH v4.1 00/10] spectre variant1 mitigations for tip/x86/pti
Hi Thomas, Please consider taking this collection of spectre-v1 mitigations through the tip/x86/pti branch for 4.16 inclusion. The review feedback has dropped off considerably, so I believe these patches are ready for -tip inclusion. However, "nl80211: sanitize array index in parse_txq_params" stands out as a patch that should get an ack from net/wireless folks before moving forward. Also a heads up that x86/pti is missing commit 75f139aaf896 "KVM: x86: Add memory barrier on vmcs field lookup", so you will hit a trivial conflict merging "kvm, x86: update spectre-v1 mitigation" from this set against latest mainline. The infrastructure includes: * __uaccess_begin_nospec: similar to __uaccess_begin this invokes 'stac', but it also includes an 'ifence'. After an 'access_ok' check has speculatively succeeded that result needs to be retired before the user pointer is de-referenced. '__get_user' can't use the pointer sanitization approach without redoing the 'access_ok' check, so per Linus [1] just use 'ifence'. * MASK_NOSPEC: an assembler macro for x86 'get_user' and syscall entry that sanitizes a user controlled pointer or array index to zero after a 'cmp %limit %val' instruction sets the CF flag. * array_ptr: When dereferencing a kernel pointer with a user controlled index, sanitize the pointer to either NULL or valid addresses under speculation to eliminate a precondition for Spectre variant1 attacks. It uses a mask generation technique that does not involve speculative control flows on either x86 or ARM64 [2]. * x86 array_ptr_mask: Achieve the same effect as the default 'array_ptr_mask' in fewer instructions. This approach does not have the same "array index and limit must be less than LONG_MAX" constraint as the default mask. * array_idx: Similar to 'array_ptr', use a mask to return a valid pointer or NULL to an array index variable. An example where we need this is the wireless driver stack where the core sanitizes user input and the actual usage of the array index is in a different compilation unit in the low-level driver. [1]: https://lkml.org/lkml/2018/1/17/929 [2]: https://www.spinics.net/lists/netdev/msg477542.html --- Dan Williams (9): asm/nospec, array_ptr: sanitize speculative array de-references x86: implement array_ptr_mask() x86: introduce __uaccess_begin_nospec and ifence x86, __get_user: use __uaccess_begin_nospec x86, get_user: use pointer masking to limit speculation x86: narrow out of bounds syscalls to sys_read under speculation vfs, fdtable: prevent bounds-check bypass via speculative execution kvm, x86: update spectre-v1 mitigation nl80211: sanitize array index in parse_txq_params Mark Rutland (1): Documentation: document array_ptr Documentation/speculation.txt | 143 + arch/x86/entry/entry_64.S |2 + arch/x86/include/asm/barrier.h| 28 +++ arch/x86/include/asm/msr.h|3 - arch/x86/include/asm/smap.h | 24 ++ arch/x86/include/asm/uaccess.h| 15 +++- arch/x86/include/asm/uaccess_32.h |6 +- arch/x86/include/asm/uaccess_64.h | 12 ++- arch/x86/kvm/vmx.c| 11 ++- arch/x86/lib/getuser.S|5 + arch/x86/lib/usercopy_32.c|8 +- include/linux/fdtable.h |7 +- include/linux/nospec.h| 65 + net/wireless/nl80211.c| 10 ++- 14 files changed, 312 insertions(+), 27 deletions(-) create mode 100644 Documentation/speculation.txt create mode 100644 include/linux/nospec.h
Re: [PATCH v4 00/10] prevent bounds-check bypass via speculative execution
On Sat, Jan 20, 2018 at 8:56 AM, Alexei Starovoitov wrote: > On Fri, Jan 19, 2018 at 10:58:44PM -0800, Dan Williams wrote: >> On Thu, Jan 18, 2018 at 4:01 PM, Dan Williams >> wrote: >> > Changes since v3 [1] >> > * Drop 'ifence_array_ptr' and associated compile-time + run-time >> > switching and just use the masking approach all the time. >> > >> > * Convert 'get_user' to use pointer sanitization via masking rather than >> > lfence. '__get_user' and associated paths still rely on >> > lfence. (Linus) >> > >> > "Basically, the rule is trivial: find all 'stac' users, and use >> >address masking if those users already integrate the limit >> >check, and lfence they don't." >> > >> > * At syscall entry sanitize the syscall number under speculation >> > to remove a user controlled pointer de-reference in kernel >> > space. (Linus) >> > >> > * Fix a raw lfence in the kvm code (added for v4.15-rc8) to use >> > 'array_ptr'. >> > >> > * Propose 'array_idx' as a way to sanitize user input that is >> > later used as an array index, but where the validation is >> > happening in a different code block than the array reference. >> > (Christian). >> > >> > * Fix grammar in speculation.txt (Kees) >> > >> > --- >> > >> > Quoting Mark's original RFC: >> > >> > "Recently, Google Project Zero discovered several classes of attack >> > against speculative execution. One of these, known as variant-1, allows >> > explicit bounds checks to be bypassed under speculation, providing an >> > arbitrary read gadget. Further details can be found on the GPZ blog [2] >> > and the Documentation patch in this series." >> > >> > A precondition of using this attack on the kernel is to get a user >> > controlled pointer de-referenced (under speculation) in privileged code. >> > The primary source of user controlled pointers in the kernel is the >> > arguments passed to 'get_user' and '__get_user'. An example of other >> > user controlled pointers are user-controlled array / pointer offsets. >> > >> > Better tooling is needed to find more arrays / pointers with user >> > controlled indices / offsets that can be converted to use 'array_ptr' or >> > 'array_idx'. A few are included in this set, and these are not expected >> > to be complete. That said, the 'get_user' protections raise the bar on >> > finding a vulnerable gadget in the kernel. >> > >> > These patches are also available via the 'nospec-v4' git branch here: >> > >> > git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v4 >> >> I've pushed out a nospec-v4.1 with the below minor cleanup, a fixup of >> the changelog for "kvm, x86: fix spectre-v1 mitigation", and added >> Paolo's ack. >> >> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v4.1 >> >> diff --git a/include/linux/nospec.h b/include/linux/nospec.h >> index 8af35be1869e..b8a9222e34d1 100644 >> --- a/include/linux/nospec.h >> +++ b/include/linux/nospec.h >> @@ -37,7 +37,7 @@ static inline unsigned long array_ptr_mask(unsigned >> long idx, unsigned long sz) >> unsigned long _i = (idx); \ >> unsigned long _mask = array_ptr_mask(_i, (sz)); \ >> \ >> - __u._ptr = _arr + (_i & _mask); \ >> + __u._ptr = _arr + _i; \ >> __u._bit &= _mask; \ >> __u._ptr; \ > > hmm. I'm not sure it's the right thing to do, since the macro > is forcing cpu to speculate subsequent load from null instead > of valid pointer. > As Linus said: " > So that __u._bit masking wasn't masking > the pointer, it was masking the value that was *read* from the > pointer, so that you could know that an invalid access returned > 0/NULL, not just the first value in the array. > " > imo just > return _arr + (_i & _mask); > is enough. No need for union games. > The cpu will speculate the load from _arr[0] if _i is out of bounds > which is the same as if user passed _i == 0 which would have passed > bounds check anyway, so I don't see any data leak from populating > cache with _arr[0] data. In-bounds access can do that just as well > without any speculation. scratch that. It's array_ptr, not array_access. The code will do if (!ptr) later, so yeah this api is fine.
Re: [PATCH v4 00/10] prevent bounds-check bypass via speculative execution
On Fri, Jan 19, 2018 at 10:58:44PM -0800, Dan Williams wrote: > On Thu, Jan 18, 2018 at 4:01 PM, Dan Williams > wrote: > > Changes since v3 [1] > > * Drop 'ifence_array_ptr' and associated compile-time + run-time > > switching and just use the masking approach all the time. > > > > * Convert 'get_user' to use pointer sanitization via masking rather than > > lfence. '__get_user' and associated paths still rely on > > lfence. (Linus) > > > > "Basically, the rule is trivial: find all 'stac' users, and use > >address masking if those users already integrate the limit > >check, and lfence they don't." > > > > * At syscall entry sanitize the syscall number under speculation > > to remove a user controlled pointer de-reference in kernel > > space. (Linus) > > > > * Fix a raw lfence in the kvm code (added for v4.15-rc8) to use > > 'array_ptr'. > > > > * Propose 'array_idx' as a way to sanitize user input that is > > later used as an array index, but where the validation is > > happening in a different code block than the array reference. > > (Christian). > > > > * Fix grammar in speculation.txt (Kees) > > > > --- > > > > Quoting Mark's original RFC: > > > > "Recently, Google Project Zero discovered several classes of attack > > against speculative execution. One of these, known as variant-1, allows > > explicit bounds checks to be bypassed under speculation, providing an > > arbitrary read gadget. Further details can be found on the GPZ blog [2] > > and the Documentation patch in this series." > > > > A precondition of using this attack on the kernel is to get a user > > controlled pointer de-referenced (under speculation) in privileged code. > > The primary source of user controlled pointers in the kernel is the > > arguments passed to 'get_user' and '__get_user'. An example of other > > user controlled pointers are user-controlled array / pointer offsets. > > > > Better tooling is needed to find more arrays / pointers with user > > controlled indices / offsets that can be converted to use 'array_ptr' or > > 'array_idx'. A few are included in this set, and these are not expected > > to be complete. That said, the 'get_user' protections raise the bar on > > finding a vulnerable gadget in the kernel. > > > > These patches are also available via the 'nospec-v4' git branch here: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v4 > > I've pushed out a nospec-v4.1 with the below minor cleanup, a fixup of > the changelog for "kvm, x86: fix spectre-v1 mitigation", and added > Paolo's ack. > > git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v4.1 > > diff --git a/include/linux/nospec.h b/include/linux/nospec.h > index 8af35be1869e..b8a9222e34d1 100644 > --- a/include/linux/nospec.h > +++ b/include/linux/nospec.h > @@ -37,7 +37,7 @@ static inline unsigned long array_ptr_mask(unsigned > long idx, unsigned long sz) > unsigned long _i = (idx); \ > unsigned long _mask = array_ptr_mask(_i, (sz)); \ > \ > - __u._ptr = _arr + (_i & _mask); \ > + __u._ptr = _arr + _i; \ > __u._bit &= _mask; \ > __u._ptr; \ hmm. I'm not sure it's the right thing to do, since the macro is forcing cpu to speculate subsequent load from null instead of valid pointer. As Linus said: " So that __u._bit masking wasn't masking the pointer, it was masking the value that was *read* from the pointer, so that you could know that an invalid access returned 0/NULL, not just the first value in the array. " imo just return _arr + (_i & _mask); is enough. No need for union games. The cpu will speculate the load from _arr[0] if _i is out of bounds which is the same as if user passed _i == 0 which would have passed bounds check anyway, so I don't see any data leak from populating cache with _arr[0] data. In-bounds access can do that just as well without any speculation.
Re: Google Summer of Code 2018 - Project ideas page for the Linux Foundation online
On Thu, Jan 18, 2018 at 10:59:14AM -0200, Till Kamppeter wrote: > Hi, > > I have set up a page for project ideas for the Linux Foundation's > participation in the Google Summer of Code 2018: > > https://wiki.linuxfoundation.org/gsoc/google-summer-code-2018 > > Please add your ideas to the sub-page of your work group. If you have > problems mail me with your project idea. > > The ideas list is in the Linux Foundation Wiki. If you want to edit, please > tell me and I give you edit rights. Looks good to me, thanks! > Please also take into account that the deadline for our application as > mentoring organization is Jan 23 and after that Google will evaluate the > applications. So have your ideas (at least most of them, ideas can be posted > up to the student application deadline) in by then to raise our chances to > get accepted. Are we supposed to sign up as mentors on the GSoC site yet? I tried and got an error saying "something went wrong". thanks, greg k-h
