| Message ID | 151520103755.32271.6819511294540882298.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
Hello! On 1/6/2018 4:10 AM, 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> > --- > drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > index 988c8857d78c..0ff34cbe2b62 100644 > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -41,6 +41,7 @@ > #include <linux/module.h> > #include <linux/etherdevice.h> > #include <linux/random.h> > +#include <linux/compiler.h> > #include <net/mac80211.h> > #include <net/cfg80211.h> > #include "hw.h" > @@ -1384,11 +1385,12 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, > const struct ieee80211_tx_queue_params *param) > { > struct ar9170 *ar = hw->priv; > + const u8 *elem; > int ret; > > mutex_lock(&ar->mutex); > - if (queue < ar->hw->queues) { > - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); > + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) { I bet this causes checkpatch.pl to complain. I don't see a dire need to upset it here, the assignment may well precede *if*... > + memcpy(&ar->edcf[*elem], param, sizeof(*param)); > ret = carl9170_set_qos(ar); > } else { > ret = -EINVAL; > MBR, Sergei
On Saturday, January 6, 2018 2:10:37 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> > --- > drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > index 988c8857d78c..0ff34cbe2b62 100644 > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -41,6 +41,7 @@ > #include <linux/module.h> > #include <linux/etherdevice.h> > #include <linux/random.h> > +#include <linux/compiler.h> > #include <net/mac80211.h> > #include <net/cfg80211.h> > #include "hw.h" > @@ -1384,11 +1385,12 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, > const struct ieee80211_tx_queue_params *param) > { > struct ar9170 *ar = hw->priv; > + const u8 *elem; > int ret; > > mutex_lock(&ar->mutex); > - if (queue < ar->hw->queues) { > - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); > + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) { > + memcpy(&ar->edcf[*elem], param, sizeof(*param)); > ret = carl9170_set_qos(ar); > } else { > ret = -EINVAL; > > About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx: The only way a user can set this in any meaningful way would be via a NL80211_CMD_SET_WIPHY netlink message. However, the value will get vetted there by cfg80211's parse_txq_params [0]. This is long before it reaches any of the *_op_conf_tx functions. 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. [0] <https://github.com/torvalds/linux/blob/master/net/wireless/nl80211.c#L2070> [1] <https://github.com/torvalds/linux/blob/master/net/mac80211/cfg.c#L2157>
> The only way a user can set this in any meaningful way would be via > a NL80211_CMD_SET_WIPHY netlink message. However, the value will get > vetted there by cfg80211's parse_txq_params [0]. This is long before Far more than a couple of hundred instructions ? The problem is that the processor will speculate that the parameter is valid and continue on that basis until the speculation resolves or it hits some other limit that stops it speculating further. That can be quite a distance on a modern x86 processor, and for all we know could be even more on some of the other CPUs involved. > it reaches any of the *_op_conf_tx functions. > > 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; > | ^^^^^^^^ Firstly it might not because the address you get as a result could be valid kernel memory. In fact your attackers wants it to be valid kernel memory since they are trying to find the value of a piece of that memory. Secondly the processor is doing this speculatively so it won't fault. It will eventually decide it went the wrong way and throw all the speculative work away - leaving footprints. It won't fault unless the speculative resolves that was the real path the code took. If it's not a performance critical path then it's better to be safe. Alan
On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter <chunkeey@gmail.com> wrote: > On Saturday, January 6, 2018 2:10:37 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> >> --- >> drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c >> index 988c8857d78c..0ff34cbe2b62 100644 >> --- a/drivers/net/wireless/ath/carl9170/main.c >> +++ b/drivers/net/wireless/ath/carl9170/main.c >> @@ -41,6 +41,7 @@ >> #include <linux/module.h> >> #include <linux/etherdevice.h> >> #include <linux/random.h> >> +#include <linux/compiler.h> >> #include <net/mac80211.h> >> #include <net/cfg80211.h> >> #include "hw.h" >> @@ -1384,11 +1385,12 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, >> const struct ieee80211_tx_queue_params *param) >> { >> struct ar9170 *ar = hw->priv; >> + const u8 *elem; >> int ret; >> >> mutex_lock(&ar->mutex); >> - if (queue < ar->hw->queues) { >> - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); >> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) { >> + memcpy(&ar->edcf[*elem], param, sizeof(*param)); >> ret = carl9170_set_qos(ar); >> } else { >> ret = -EINVAL; >> >> > About the "queue" in carl9170_op_conf_tx, p54_conf_tx and cw1200_conf_tx: > > The only way a user can set this in any meaningful way would be via > a NL80211_CMD_SET_WIPHY netlink message. However, the value will get > vetted there by cfg80211's parse_txq_params [0]. This is long before > it reaches any of the *_op_conf_tx functions. > > 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.
On Saturday, January 6, 2018 4:06:21 PM CET Alan Cox wrote: > > The only way a user can set this in any meaningful way would be via > > a NL80211_CMD_SET_WIPHY netlink message. However, the value will get > > vetted there by cfg80211's parse_txq_params [0]. This is long before > > Far more than a couple of hundred instructions ? Well, the user would have to send a netlink message each time. So cfg80211 can parse it (this is where the initial "if queue >= 4 " check happen). So the CPU would have to continue through and into rdev_set_txq_params() to get to mac80211's ieee80211_set_txq_params(). Then pass through that before gets to the driver's op_tx_conf. Once there the driver code aquires a mutex_lock too before it gets to check the queue value again. Is this enough and how would the mutex_lock fit in there? Or can the CPU skip past this as well? > The problem is that the processor will speculate that the parameter > is valid and continue on that basis until the speculation resolves > or it hits some other limit that stops it speculating further. > That can be quite a distance on a modern x86 processor, and for all > we know could be even more on some of the other CPUs involved. > > it reaches any of the *_op_conf_tx functions. > > > > 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; > > | ^^^^^^^^ > > Firstly it might not because the address you get as a result could be > valid kernel memory. In fact your attackers wants it to be valid kernel > memory since they are trying to find the value of a piece of that memory. > > Secondly the processor is doing this speculatively so it won't fault. It > will eventually decide it went the wrong way and throw all the > speculative work away - leaving footprints. It won't fault unless the > speculative resolves that was the real path the code took. > > If it's not a performance critical path then it's better to be safe. Thank you for reading the canary too. Christian
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 988c8857d78c..0ff34cbe2b62 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -41,6 +41,7 @@ #include <linux/module.h> #include <linux/etherdevice.h> #include <linux/random.h> +#include <linux/compiler.h> #include <net/mac80211.h> #include <net/cfg80211.h> #include "hw.h" @@ -1384,11 +1385,12 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw, const struct ieee80211_tx_queue_params *param) { struct ar9170 *ar = hw->priv; + const u8 *elem; int ret; mutex_lock(&ar->mutex); - if (queue < ar->hw->queues) { - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param)); + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) { + memcpy(&ar->edcf[*elem], param, sizeof(*param)); ret = carl9170_set_qos(ar); } else { ret = -EINVAL;