From mboxrd@z Thu Jan 1 00:00:00 1970 From: Johannes Berg Subject: Re: [PATCH v4 10/10] nl80211: sanitize array index in parse_txq_params Date: Sun, 21 Jan 2018 11:37:43 +0100 Message-ID: <1516531063.2508.2.camel@sipsolutions.net> References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: Received: from s3.sipsolutions.net ([144.76.63.242]:48820 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751092AbeAUKhv (ORCPT ); Sun, 21 Jan 2018 05:37:51 -0500 In-Reply-To: <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dan Williams , linux-kernel@vger.kernel.org Cc: linux-arch@vger.kernel.org, akpm@linux-foundation.org, kernel-hardening@lists.openwall.com, gregkh@linuxfoundation.org, Christian Lamparter , linux-wireless@vger.kernel.org, tglx@linutronix.de, torvalds@linux-foundation.org, "David S. Miller" , Elena Reshetova , alan@linux.intel.com On Thu, 2018-01-18 at 16:02 -0800, Dan Williams wrote: > 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). Sorry, I didn't realize you were waiting for me to review. LGTM. Reviewed-by: Johannes Berg > 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 f841c11f3f1f..8af35be1869e 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 2b3dbcd40e46..202cb1dc03ee 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; > } > > >