From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <1516531063.2508.2.camel@sipsolutions.net> (sfid-20180121_113806_147231_156FC166) Subject: Re: [PATCH v4 10/10] nl80211: sanitize array index in parse_txq_params From: Johannes Berg 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 Date: Sun, 21 Jan 2018 11:37:43 +0100 In-Reply-To: <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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; > } > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x225Oh2G4bomB8fzrAkRI4D7OgRT5U+eMNTuqWOfVcX9OJ+QpRYSvnYvzgMxiT+SN9dMMoxk/ ARC-Seal: i=1; a=rsa-sha256; t=1516531069; cv=none; d=google.com; s=arc-20160816; b=CW5wI4O35LUZIlDxKd0M0CDfC4WD8HV0uXk9gIaEYzPetenRxIH3Cxx45v5QXeuulX BYb0vSx+QoJFts/2OOZNWz4XPEUgjllyc+NzLR6YXK3TKwhF6TNvzXduYvpTJnKOy/C1 2PFXPeypmcSYda6ZceaNWsiITBzzRcfXVBbFmO2Id9f8X67aCSbVJu4PJwMFercASH8q 1TanA/DCEFSKrQqlWlnX4WigH/KUDuk/hfg+6iANZM5SUNzX9THMuu3CBhltTwwHxS6h qcBKNNEpHSWCFQHPdcp3rdEaiMGHcIqv78Dmhah/Q+gXvR1GahxoH62332t4A43eeDGu tkng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:references:in-reply-to:date :cc:to:from:subject:message-id:arc-authentication-results; bh=FI4lSw++UQHcDzgeqi6sweIdVjyjwSuo3n69yoooVAM=; b=k8N6b1v/Rp5K1gNEL3vOrSlPSYda7YdxkKNLqS33wfHqGOZh7qwCMhg3BonvuBfwOw Q94pL5kAEBdvlqX+rGVlB1omT5nAViws3RzpO4J78S1pXoVnFD6P4AUWTheyRAneHWgK IwKbpjron7swmEUJOHN5Q4fVTr/ozNTfdU84ZumGuPjXjY4c7cxwG5Vc0QTQdGAD9UfV xbRcCm7blRMsie4SfoUXB0FtV8rb69jgasH0X9+rTx3L+OMfnIavkNZS7xRdRPxwW97o XMXMrAc7D01jN0CaVvuto5nj9TgbqNBcxQHRMrLMf+QmI4bV8SizAS35xuDK2yIEtgD+ Ioag== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of johannes@sipsolutions.net designates 2a01:4f8:191:72ef::2 as permitted sender) smtp.mailfrom=johannes@sipsolutions.net Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of johannes@sipsolutions.net designates 2a01:4f8:191:72ef::2 as permitted sender) smtp.mailfrom=johannes@sipsolutions.net Message-ID: <1516531063.2508.2.camel@sipsolutions.net> Subject: Re: [PATCH v4 10/10] nl80211: sanitize array index in parse_txq_params From: Johannes Berg 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 Date: Sun, 21 Jan 2018 11:37:43 +0100 In-Reply-To: <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.2-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1589977502140372755?= X-GMAIL-MSGID: =?utf-8?q?1590198082211697781?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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; > } > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <1516531063.2508.2.camel@sipsolutions.net> From: Johannes Berg Date: Sun, 21 Jan 2018 11:37:43 +0100 In-Reply-To: <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> References: <151632009605.21271.11304291057104672116.stgit@dwillia2-desk3.amr.corp.intel.com> <151632015637.21271.10452415430644852207.stgit@dwillia2-desk3.amr.corp.intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [kernel-hardening] Re: [PATCH v4 10/10] nl80211: sanitize array index in parse_txq_params 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 List-ID: 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; > } > > >