From: Christian Lamparter <chunkeey@gmail.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-arch@vger.kernel.org, kernel-hardening@lists.openwall.com,
Netdev <netdev@vger.kernel.org>,
Linux Wireless List <linux-wireless@vger.kernel.org>,
Elena Reshetova <elena.reshetova@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
Kalle Valo <kvalo@codeaurora.org>,
Alan Cox <alan@linux.intel.com>
Subject: Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution
Date: Fri, 12 Jan 2018 21:01:59 +0100 [thread overview]
Message-ID: <6067453.9oC6tc2YFn@debian64> (raw)
In-Reply-To: <CAPcyv4gne66ic3E-Y+pLi4QZdVeeeVmvayVPA3Jt1gyUqpaF9g@mail.gmail.com>
On Friday, January 12, 2018 7:39:50 PM CET Dan Williams wrote:
> On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter <chunkeey@gmail.com> wrote:
> > On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
> >> Static analysis reports that 'queue' may be a user controlled value that
> >> is used as a data dependency to read from the 'ar9170_qmap' array. In
> >> order to avoid potential leaks of kernel memory values, block
> >> speculative execution of the instruction stream that could issue reads
> >> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> >> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> >> 'ar->edcf' array.
> >>
> >> Based on an original patch by Elena Reshetova.
> >>
> >> Cc: Christian Lamparter <chunkeey@googlemail.com>
> >> Cc: Kalle Valo <kvalo@codeaurora.org>
> >> Cc: linux-wireless@vger.kernel.org
> >> Cc: netdev@vger.kernel.org
> >> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> > This patch (and p54, cw1200) look like the same patch?!
> > Can you tell me what happend to:
> >
> > On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
> >> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter <chunkeey@gmail.com> wrote:
> >> > And Furthermore a invalid queue (param->ac) would cause a crash in
> >> > this line in mac80211 before it even reaches the driver [1]:
> >> > | sdata->tx_conf[params->ac] = p;
> >> > | ^^^^^^^^
> >> > | if (drv_conf_tx(local, sdata, >>>> params->ac <<<<, &p)) {
> >> > | ^^ (this is a wrapper for the *_op_conf_tx)
> >> >
> >> > I don't think these chin-up exercises are needed.
> >>
> >> Quite the contrary, you've identified a better place in the call stack
> >> to sanitize the input and disable speculation. Then we can kill the
> >> whole class of the wireless driver reports at once it seems.
> > <https://www.spinics.net/lists/netdev/msg476353.html>
>
> I didn't see where ac is being validated against the driver specific
> 'queues' value in that earlier patch.
The link to the check is right there in the earlier post. It's in
parse_txq_params():
<https://github.com/torvalds/linux/blob/master/net/wireless/nl80211.c#L2070>
| if (txq_params->ac >= NL80211_NUM_ACS)
| return -EINVAL;
NL80211_NUM_ACS is 4
<http://elixir.free-electrons.com/linux/v4.15-rc7/source/include/uapi/linux/nl80211.h#L3748>
This check was added ever since mac80211's ieee80211_set_txq_params():
| sdata->tx_conf[params->ac] = p;
For cw1200: the driver just sets the dev->queue to 4.
In carl9170 dev->queues is set to __AR9170_NUM_TXQ and
p54 uses P54_QUEUE_AC_NUM.
Both __AR9170_NUM_TXQ and P54_QUEUE_AC_NUM are 4.
And this is not going to change since all drivers
have to follow mac80211's queue API:
<https://wireless.wiki.kernel.org/en/developers/documentation/mac80211/queues>
Some background:
In the old days (linux 2.6 and early 3.x), the parse_txq_params() function did
not verify the "queue" value. That's why these drivers had to do it.
Here's the relevant code from 2.6.39:
<http://elixir.free-electrons.com/linux/v2.6.39/source/net/wireless/nl80211.c#L879>
You'll notice that the check is missing there.
Here's mac80211's ieee80211_set_txq_params for reference:
<http://elixir.free-electrons.com/linux/v2.6.39/source/net/mac80211/cfg.c#L1197>
However over time, the check in the driver has become redundant.
> > Anyway, I think there's an easy way to solve this: remove the
> > "if (queue < ar->hw->queues)" check altogether. It's no longer needed
> > anymore as the "queue" value is validated long before the driver code
> > gets called.
> >
> > And from my understanding, this will fix the "In this case
> > the value of 'ar9170_qmap[queue]' is immediately reused as an index to
> > the 'ar->edcf' array." gripe your tool complains about.
> >
> > This is here's a quick test-case for carl9170.:
> > ---
> > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> > index 988c8857d78c..2d3afb15bb62 100644
> > --- a/drivers/net/wireless/ath/carl9170/main.c
> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> > int ret;
> >
> > mutex_lock(&ar->mutex);
> > - if (queue < ar->hw->queues) {
> > - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> > - ret = carl9170_set_qos(ar);
> > - } else {
> > - ret = -EINVAL;
> > - }
> > -
> > + memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> > + ret = carl9170_set_qos(ar);
> > mutex_unlock(&ar->mutex);
> > return ret;
> > }
> > ---
> > What does your tool say about this?
>
> If you take away the 'if' then I don't the tool will report on this.
>
> > (If necessary, the "queue" value could be even sanitized with a
> > queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)
>
> That is what array_ptr() is doing in a more sophisticated way.
I think it's a very roundabout way :D. In any case the queue %= ...
could also be replaced by:
BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != NL80211_NUM_ACS));
(And the equivalent for p54, cw1200)
next prev parent reply other threads:[~2018-01-12 20:02 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-12 0:46 [PATCH v2 00/19] prevent bounds-check bypass via speculative execution Dan Williams
2018-01-12 0:46 ` [PATCH v2 01/19] Documentation: document array_ptr Dan Williams
2018-01-12 10:38 ` Geert Uytterhoeven
2018-01-16 21:01 ` Kees Cook
2018-01-12 0:46 ` [PATCH v2 02/19] arm64: implement ifence_array_ptr() Dan Williams
2018-01-12 0:46 ` [PATCH v2 03/19] arm: " Dan Williams
2018-01-12 0:46 ` [PATCH v2 04/19] x86: implement ifence() Dan Williams
2018-01-12 2:27 ` Eric W. Biederman
2018-01-12 3:39 ` Dan Williams
2018-01-12 0:46 ` [PATCH v2 05/19] x86: implement ifence_array_ptr() and array_ptr_mask() Dan Williams
2018-01-12 0:46 ` [PATCH v2 06/19] asm-generic/barrier: mask speculative execution flows Dan Williams
2018-01-12 2:42 ` Eric W. Biederman
2018-01-12 9:12 ` Peter Zijlstra
2018-01-13 0:41 ` Dan Williams
2018-01-15 8:46 ` Peter Zijlstra
2018-01-12 0:47 ` [PATCH v2 07/19] x86: introduce __uaccess_begin_nospec and ASM_IFENCE Dan Williams
2018-01-12 17:51 ` Josh Poimboeuf
2018-01-12 18:21 ` Dan Williams
2018-01-12 18:58 ` Josh Poimboeuf
2018-01-12 19:26 ` Dan Williams
2018-01-12 20:01 ` Linus Torvalds
2018-01-12 20:41 ` Josh Poimboeuf
2018-01-12 0:47 ` [PATCH v2 08/19] x86: use __uaccess_begin_nospec and ASM_IFENCE in get_user paths Dan Williams
2018-01-12 1:11 ` Linus Torvalds
2018-01-12 1:14 ` Dan Williams
2018-01-12 0:47 ` [PATCH v2 09/19] ipv6: prevent bounds-check bypass via speculative execution Dan Williams
2018-01-12 0:47 ` [PATCH v2 10/19] ipv4: " Dan Williams
2018-01-12 7:59 ` Greg KH
2018-01-12 18:47 ` Dan Williams
2018-01-13 8:56 ` Greg KH
2018-01-12 0:47 ` [PATCH v2 11/19] vfs, fdtable: " Dan Williams
2018-01-12 0:47 ` [PATCH v2 12/19] userns: " Dan Williams
2018-01-12 0:47 ` [PATCH v2 13/19] udf: " Dan Williams
2018-01-15 10:32 ` Jan Kara
2018-01-15 17:49 ` Dan Williams
2018-01-12 0:47 ` [PATCH v2 14/19] [media] uvcvideo: " Dan Williams
2018-08-06 21:40 ` Laurent Pinchart
2018-01-12 0:47 ` [PATCH v2 15/19] carl9170: " Dan Williams
2018-01-12 14:42 ` Christian Lamparter
2018-01-12 18:39 ` Dan Williams
2018-01-12 20:01 ` Christian Lamparter [this message]
2018-01-12 23:05 ` Dan Williams
2018-01-12 0:47 ` [PATCH v2 16/19] p54: " Dan Williams
2018-01-12 0:47 ` [PATCH v2 17/19] qla2xxx: " Dan Williams
2018-01-12 1:19 ` James Bottomley
2018-01-12 5:38 ` Dan Williams
2018-01-12 6:05 ` James Bottomley
2018-01-12 0:48 ` [PATCH v2 18/19] cw1200: " Dan Williams
2018-01-12 0:48 ` [PATCH v2 19/19] net: mpls: " Dan Williams
2018-01-12 1:19 ` [PATCH v2 00/19] " Linus Torvalds
2018-01-12 1:41 ` Dan Williams
2018-01-18 13:18 ` Will Deacon
2018-01-18 16:58 ` Dan Williams
2018-01-18 17:05 ` Will Deacon
2018-01-18 21:41 ` Laurent Pinchart
2018-01-13 0:15 ` Tony Luck
2018-01-13 18:51 ` Linus Torvalds
2018-01-16 19:21 ` Tony Luck
2018-01-12 10:02 ` Russell King - ARM Linux
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6067453.9oC6tc2YFn@debian64 \
--to=chunkeey@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=alan@linux.intel.com \
--cc=dan.j.williams@intel.com \
--cc=elena.reshetova@intel.com \
--cc=kernel-hardening@lists.openwall.com \
--cc=kvalo@codeaurora.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).