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 [1]
and the Documentation patch in this series."
This series incorporates Mark Rutland's latest api and adds the x86
specific implementation of nospec_barrier. The
nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
wide analysis performed by Elena Reshetova to address static analysis
reports where speculative execution on a userspace controlled value
could bypass a bounds check. The patches address a precondition for the
attack discussed in the Spectre paper [2].
A consideration worth noting for reviewing these patches is to weigh the
dramatic cost of being wrong about whether a given report is exploitable
vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
lets make the bar for applying these patches be "can you prove that the
bounds check bypass is *not* exploitable". Consider that the Spectre
paper reports one example of a speculation window being ~180 cycles.
Note that there is also a proposal from Linus, array_access [3], that
attempts to quash speculative execution past a bounds check without
introducing an lfence instruction. That may be a future optimization
possibility that is compatible with this api, but it would appear to
need guarantees from the compiler that it is not clear the kernel can
rely on at this point. It is also not clear that it would be a
significant performance win vs lfence.
These patches also will also be available via the 'nospec' git branch
here:
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec
[1]: https://googleprojectzero.blogspot.co.uk/2018/01/reading-privileged-memory-with-side.html
[2]: https://spectreattack.com/spectre.pdf
[3]: https://marc.info/?l=linux-kernel&m=151510446027625&w=2
---
Andi Kleen (1):
x86, barrier: stop speculation for failed access_ok
Dan Williams (13):
x86: implement nospec_barrier()
[media] uvcvideo: prevent bounds-check bypass via speculative execution
carl9170: prevent bounds-check bypass via speculative execution
p54: prevent bounds-check bypass via speculative execution
qla2xxx: prevent bounds-check bypass via speculative execution
cw1200: prevent bounds-check bypass via speculative execution
Thermal/int340x: prevent bounds-check bypass via speculative execution
ipv6: prevent bounds-check bypass via speculative execution
ipv4: prevent bounds-check bypass via speculative execution
vfs, fdtable: prevent bounds-check bypass via speculative execution
net: mpls: prevent bounds-check bypass via speculative execution
udf: prevent bounds-check bypass via speculative execution
userns: prevent bounds-check bypass via speculative execution
Mark Rutland (4):
asm-generic/barrier: add generic nospec helpers
Documentation: document nospec helpers
arm64: implement nospec_ptr()
arm: implement nospec_ptr()
Documentation/speculation.txt | 166 ++++++++++++++++++++
arch/arm/include/asm/barrier.h | 75 +++++++++
arch/arm64/include/asm/barrier.h | 55 +++++++
arch/x86/include/asm/barrier.h | 6 +
arch/x86/include/asm/uaccess.h | 17 ++
drivers/media/usb/uvc/uvc_v4l2.c | 7 +
drivers/net/wireless/ath/carl9170/main.c | 6 -
drivers/net/wireless/intersil/p54/main.c | 8 +
drivers/net/wireless/st/cw1200/sta.c | 10 +
drivers/net/wireless/st/cw1200/wsm.h | 4
drivers/scsi/qla2xxx/qla_mr.c | 15 +-
.../thermal/int340x_thermal/int340x_thermal_zone.c | 14 +-
fs/udf/misc.c | 39 +++--
include/asm-generic/barrier.h | 68 ++++++++
include/linux/fdtable.h | 5 -
kernel/user_namespace.c | 10 -
net/ipv4/raw.c | 9 +
net/ipv6/raw.c | 9 +
net/mpls/af_mpls.c | 12 +
19 files changed, 466 insertions(+), 69 deletions(-)
create mode 100644 Documentation/speculation.txt
* Alan Cox <[email protected]> wrote:
> On Mon, 8 Jan 2018 11:08:36 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > On Fri, Jan 05, 2018 at 10:30:16PM -0800, Dan Williams wrote:
> > > On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
> > > > In at least one place (mpls) you are patching a fast path. Compile out
> > > > or don't load mpls by all means. But it is not acceptable to change the
> > > > fast path without even considering performance.
> > >
> > > Performance matters greatly, but I need help to identify a workload
> > > that is representative for this fast path to see what, if any, impact
> > > is incurred. Even better is a review that says "nope, 'index' is not
> > > subject to arbitrary userspace control at this point, drop the patch."
> >
> > I think we're focussing a little too much on pure userspace. That is, we
> > should be saying under the attackers control. Inbound network packets
> > could equally be under the attackers control.
>
> Inbound network packets don't come with a facility to read back and do
> cache timimg. [...]
But the reply packets can be measured on the sending side, and the total delay
timing would thus carry the timing information.
Yes, a lot of noise gets added that way if we think 'packet goes through the
Internet' - but with gigabit local network access or even through localhost
access a lot of noise can be removed as well.
It's not as dangerous as a near instantaneous local attack, but 'needs a day of
runtime to brute-force through localhost or 10GigE' is still worrying in many
real-world security contexts.
So I concur with Peter that we should generally consider making all of our
responses to external data (maybe with the exception of pigeon post messages)
Spectre-safe.
Thanks,
Ingo
Static analysis reports that 'queue' may be a user controlled value that
is used as a data dependency to read from the 'priv->qos_params' 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 'priv->qos_params[queue]'.
Based on an original patch by Elena Reshetova.
Cc: Christian Lamparter <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Elena Reshetova <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/net/wireless/intersil/p54/main.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
index ab6d39e12069..85c9cbee35fc 100644
--- a/drivers/net/wireless/intersil/p54/main.c
+++ b/drivers/net/wireless/intersil/p54/main.c
@@ -20,6 +20,7 @@
#include <linux/firmware.h>
#include <linux/etherdevice.h>
#include <linux/module.h>
+#include <linux/compiler.h>
#include <net/mac80211.h>
@@ -411,12 +412,13 @@ static int p54_conf_tx(struct ieee80211_hw *dev,
const struct ieee80211_tx_queue_params *params)
{
struct p54_common *priv = dev->priv;
+ struct p54_edcf_queue_param *p54_q;
int ret;
mutex_lock(&priv->conf_mutex);
- if (queue < dev->queues) {
- P54_SET_QUEUE(priv->qos_params[queue], params->aifs,
- params->cw_min, params->cw_max, params->txop);
+ if ((p54_q = nospec_array_ptr(priv->qos_params, queue, dev->queues))) {
+ P54_SET_QUEUE(p54_q[0], params->aifs, params->cw_min,
+ params->cw_max, params->txop);
ret = p54_set_edcf(priv);
} else
ret = -EINVAL;
On Fri, Jan 05, 2018 at 10:30:16PM -0800, Dan Williams wrote:
> On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
> > In at least one place (mpls) you are patching a fast path. Compile out
> > or don't load mpls by all means. But it is not acceptable to change the
> > fast path without even considering performance.
>
> Performance matters greatly, but I need help to identify a workload
> that is representative for this fast path to see what, if any, impact
> is incurred. Even better is a review that says "nope, 'index' is not
> subject to arbitrary userspace control at this point, drop the patch."
I think we're focussing a little too much on pure userspace. That is, we
should be saying under the attackers control. Inbound network packets
could equally be under the attackers control.
Sure, userspace is the most direct and highest bandwidth one, but I
think we should treat all (kernel) external values with the same
paranoia.
On Tue, 9 Jan 2018, Josh Poimboeuf wrote:
> On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
> > On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <[email protected]> wrote:
> > > On Fri, 5 Jan 2018, Dan Williams wrote:
> > >
> > > [ ... snip ... ]
> > >> Andi Kleen (1):
> > >> x86, barrier: stop speculation for failed access_ok
> > >>
> > >> Dan Williams (13):
> > >> x86: implement nospec_barrier()
> > >> [media] uvcvideo: prevent bounds-check bypass via speculative execution
> > >> carl9170: prevent bounds-check bypass via speculative execution
> > >> p54: prevent bounds-check bypass via speculative execution
> > >> qla2xxx: prevent bounds-check bypass via speculative execution
> > >> cw1200: prevent bounds-check bypass via speculative execution
> > >> Thermal/int340x: prevent bounds-check bypass via speculative execution
> > >> ipv6: prevent bounds-check bypass via speculative execution
> > >> ipv4: prevent bounds-check bypass via speculative execution
> > >> vfs, fdtable: prevent bounds-check bypass via speculative execution
> > >> net: mpls: prevent bounds-check bypass via speculative execution
> > >> udf: prevent bounds-check bypass via speculative execution
> > >> userns: prevent bounds-check bypass via speculative execution
> > >>
> > >> Mark Rutland (4):
> > >> asm-generic/barrier: add generic nospec helpers
> > >> Documentation: document nospec helpers
> > >> arm64: implement nospec_ptr()
> > >> arm: implement nospec_ptr()
> > >
> > > So considering the recent publication of [1], how come we all of a sudden
> > > don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
> > > LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
> > >
> > > Is this going to be handled in eBPF in some other way?
> > >
> > > Without that in place, and considering Jann Horn's paper, it would seem
> > > like PTI doesn't really lock it down fully, right?
> >
> > Here is the latest (v3) bpf fix:
> >
> > https://patchwork.ozlabs.org/patch/856645/
> >
> > I currently have v2 on my 'nospec' branch and will move that to v3 for
> > the next update, unless it goes upstream before then.
Daniel, I guess you're planning to send this still for 4.15?
> That patch seems specific to CONFIG_BPF_SYSCALL. Is the bpf() syscall
> the only attack vector? Or are there other ways to run bpf programs
> that we should be worried about?
Seems like Alexei is probably the only person in the whole universe who
isn't CCed here ... let's fix that.
Thanks,
--
Jiri Kosina
SUSE Labs
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 <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Elena Reshetova <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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;
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
> 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
Dan Williams <[email protected]> writes:
> 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 [1]
> and the Documentation patch in this series."
>
> This series incorporates Mark Rutland's latest api and adds the x86
> specific implementation of nospec_barrier. The
> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
> wide analysis performed by Elena Reshetova to address static analysis
> reports where speculative execution on a userspace controlled value
> could bypass a bounds check. The patches address a precondition for the
> attack discussed in the Spectre paper [2].
Please expand this.
It is not clear what the static analysis is looking for. Have a clear
description of what is being fixed is crucial for allowing any of these
changes.
For the details given in the change description what I read is magic
changes because a magic process says this code is vunlerable.
Given the similarities in the code that is being patched to many other
places in the kernel it is not at all clear that this small set of
changes is sufficient for any purpose.
> A consideration worth noting for reviewing these patches is to weigh the
> dramatic cost of being wrong about whether a given report is exploitable
> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
> lets make the bar for applying these patches be "can you prove that the
> bounds check bypass is *not* exploitable". Consider that the Spectre
> paper reports one example of a speculation window being ~180 cycles.
> Note that there is also a proposal from Linus, array_access [3], that
> attempts to quash speculative execution past a bounds check without
> introducing an lfence instruction. That may be a future optimization
> possibility that is compatible with this api, but it would appear to
> need guarantees from the compiler that it is not clear the kernel can
> rely on at this point. It is also not clear that it would be a
> significant performance win vs lfence.
It is also not clear that these changes fix anything, or are in any
sense correct for the problem they are trying to fix as the problem
is not clearly described.
In at least one place (mpls) you are patching a fast path. Compile out
or don't load mpls by all means. But it is not acceptable to change the
fast path without even considering performance.
So because the description sucks, and the due diligence is not there.
Nacked-by: "Eric W. Biederman" <[email protected]>
to the series.
>
> These patches also will also be available via the 'nospec' git branch
> here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec
>
> [1]: https://googleprojectzero.blogspot.co.uk/2018/01/reading-privileged-memory-with-side.html
> [2]: https://spectreattack.com/spectre.pdf
> [3]: https://marc.info/?l=linux-kernel&m=151510446027625&w=2
>
> ---
>
> Andi Kleen (1):
> x86, barrier: stop speculation for failed access_ok
>
> Dan Williams (13):
> x86: implement nospec_barrier()
> [media] uvcvideo: prevent bounds-check bypass via speculative execution
> carl9170: prevent bounds-check bypass via speculative execution
> p54: prevent bounds-check bypass via speculative execution
> qla2xxx: prevent bounds-check bypass via speculative execution
> cw1200: prevent bounds-check bypass via speculative execution
> Thermal/int340x: prevent bounds-check bypass via speculative execution
> ipv6: prevent bounds-check bypass via speculative execution
> ipv4: prevent bounds-check bypass via speculative execution
> vfs, fdtable: prevent bounds-check bypass via speculative execution
> net: mpls: prevent bounds-check bypass via speculative execution
> udf: prevent bounds-check bypass via speculative execution
> userns: prevent bounds-check bypass via speculative execution
>
> Mark Rutland (4):
> asm-generic/barrier: add generic nospec helpers
> Documentation: document nospec helpers
> arm64: implement nospec_ptr()
> arm: implement nospec_ptr()
>
> Documentation/speculation.txt | 166 ++++++++++++++++++++
> arch/arm/include/asm/barrier.h | 75 +++++++++
> arch/arm64/include/asm/barrier.h | 55 +++++++
> arch/x86/include/asm/barrier.h | 6 +
> arch/x86/include/asm/uaccess.h | 17 ++
> drivers/media/usb/uvc/uvc_v4l2.c | 7 +
> drivers/net/wireless/ath/carl9170/main.c | 6 -
> drivers/net/wireless/intersil/p54/main.c | 8 +
> drivers/net/wireless/st/cw1200/sta.c | 10 +
> drivers/net/wireless/st/cw1200/wsm.h | 4
> drivers/scsi/qla2xxx/qla_mr.c | 15 +-
> .../thermal/int340x_thermal/int340x_thermal_zone.c | 14 +-
> fs/udf/misc.c | 39 +++--
> include/asm-generic/barrier.h | 68 ++++++++
> include/linux/fdtable.h | 5 -
> kernel/user_namespace.c | 10 -
> net/ipv4/raw.c | 9 +
> net/ipv6/raw.c | 9 +
> net/mpls/af_mpls.c | 12 +
> 19 files changed, 466 insertions(+), 69 deletions(-)
> create mode 100644 Documentation/speculation.txt
> It sounds like Coverity was used to produce these patches? If so, is
> there a plan to have smatch (hey Dan) or other open source static
> analysis tool be possibly enhanced to do a similar type of work?
I'd love for that to happen; the tricky part is being able to have even a
sort of sensible concept of "trusted" vs "untrusted" value...
if you look at a very small window of code, that does not work well;
you likely need to even look (as tool) across .c file boundaries
On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter <[email protected]> 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 <[email protected]>
>> Cc: Kalle Valo <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Elena Reshetova <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>> ---
>> 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 Sat, Jan 6, 2018 at 11:37 AM, Dan Williams <[email protected]> wrote:
> On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <[email protected]> wrote:
>> 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 [1]
>> and the Documentation patch in this series."
>>
>> This series incorporates Mark Rutland's latest api and adds the x86
>> specific implementation of nospec_barrier. The
>> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
>> wide analysis performed by Elena Reshetova to address static analysis
>> reports where speculative execution on a userspace controlled value
>> could bypass a bounds check. The patches address a precondition for the
>> attack discussed in the Spectre paper [2].
>>
>> A consideration worth noting for reviewing these patches is to weigh the
>> dramatic cost of being wrong about whether a given report is exploitable
>> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
>> lets make the bar for applying these patches be "can you prove that the
>> bounds check bypass is *not* exploitable". Consider that the Spectre
>> paper reports one example of a speculation window being ~180 cycles.
>>
>> Note that there is also a proposal from Linus, array_access [3], that
>> attempts to quash speculative execution past a bounds check without
>> introducing an lfence instruction. That may be a future optimization
>> possibility that is compatible with this api, but it would appear to
>> need guarantees from the compiler that it is not clear the kernel can
>> rely on at this point. It is also not clear that it would be a
>> significant performance win vs lfence.
>>
>> These patches also will also be available via the 'nospec' git branch
>> here:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec
>
> It appears that git.kernel.org has not mirrored out the new branch. In
> the meantime here's an alternative location:
>
> https://github.com/djbw/linux.git nospec
>
> If there are updates to these patches they will appear in nospec-v2,
> nospec-v3, etc... branches.
For completeness I appended the bpf fix [1] to the git branch.
https://lwn.net/Articles/743288/
Le 01/05/18 à 17:09, Dan Williams a écrit :
> 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 [1]
> and the Documentation patch in this series."
>
> This series incorporates Mark Rutland's latest api and adds the x86
> specific implementation of nospec_barrier. The
> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
> wide analysis performed by Elena Reshetova to address static analysis
> reports where speculative execution on a userspace controlled value
> could bypass a bounds check. The patches address a precondition for the
> attack discussed in the Spectre paper [2].
>
> A consideration worth noting for reviewing these patches is to weigh the
> dramatic cost of being wrong about whether a given report is exploitable
> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
> lets make the bar for applying these patches be "can you prove that the
> bounds check bypass is *not* exploitable". Consider that the Spectre
> paper reports one example of a speculation window being ~180 cycles.
>
> Note that there is also a proposal from Linus, array_access [3], that
> attempts to quash speculative execution past a bounds check without
> introducing an lfence instruction. That may be a future optimization
> possibility that is compatible with this api, but it would appear to
> need guarantees from the compiler that it is not clear the kernel can
> rely on at this point. It is also not clear that it would be a
> significant performance win vs lfence.
>
> These patches also will also be available via the 'nospec' git branch
> here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec
Although I suppose -stable and distribution maintainers will keep a
close eye on these patches, is there a particular reason why they don't
include the relevant CVE number in their commit messages?
It sounds like Coverity was used to produce these patches? If so, is
there a plan to have smatch (hey Dan) or other open source static
analysis tool be possibly enhanced to do a similar type of work?
Thanks!
--
Florian
On Mon, Jan 08, 2018 at 11:43:42AM +0000, Alan Cox wrote:
> On Mon, 8 Jan 2018 11:08:36 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > On Fri, Jan 05, 2018 at 10:30:16PM -0800, Dan Williams wrote:
> > > On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
> > > > In at least one place (mpls) you are patching a fast path. Compile out
> > > > or don't load mpls by all means. But it is not acceptable to change the
> > > > fast path without even considering performance.
> > >
> > > Performance matters greatly, but I need help to identify a workload
> > > that is representative for this fast path to see what, if any, impact
> > > is incurred. Even better is a review that says "nope, 'index' is not
> > > subject to arbitrary userspace control at this point, drop the patch."
> >
> > I think we're focussing a little too much on pure userspace. That is, we
> > should be saying under the attackers control. Inbound network packets
> > could equally be under the attackers control.
>
> Inbound network packets don't come with a facility to read back and do
> cache timimg.
But could they not be used in conjunction with a local task to prime the
stuff?
On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <[email protected]> wrote:
> 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 [1]
> and the Documentation patch in this series."
>
> This series incorporates Mark Rutland's latest api and adds the x86
> specific implementation of nospec_barrier. The
> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
> wide analysis performed by Elena Reshetova to address static analysis
> reports where speculative execution on a userspace controlled value
> could bypass a bounds check. The patches address a precondition for the
> attack discussed in the Spectre paper [2].
>
> A consideration worth noting for reviewing these patches is to weigh the
> dramatic cost of being wrong about whether a given report is exploitable
> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
> lets make the bar for applying these patches be "can you prove that the
> bounds check bypass is *not* exploitable". Consider that the Spectre
> paper reports one example of a speculation window being ~180 cycles.
>
> Note that there is also a proposal from Linus, array_access [3], that
> attempts to quash speculative execution past a bounds check without
> introducing an lfence instruction. That may be a future optimization
> possibility that is compatible with this api, but it would appear to
> need guarantees from the compiler that it is not clear the kernel can
> rely on at this point. It is also not clear that it would be a
> significant performance win vs lfence.
>
> These patches also will also be available via the 'nospec' git branch
> here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec
It appears that git.kernel.org has not mirrored out the new branch. In
the meantime here's an alternative location:
https://github.com/djbw/linux.git nospec
If there are updates to these patches they will appear in nospec-v2,
nospec-v3, etc... branches.
On Fri, 5 Jan 2018, Dan Williams wrote:
[ ... snip ... ]
> Andi Kleen (1):
> x86, barrier: stop speculation for failed access_ok
>
> Dan Williams (13):
> x86: implement nospec_barrier()
> [media] uvcvideo: prevent bounds-check bypass via speculative execution
> carl9170: prevent bounds-check bypass via speculative execution
> p54: prevent bounds-check bypass via speculative execution
> qla2xxx: prevent bounds-check bypass via speculative execution
> cw1200: prevent bounds-check bypass via speculative execution
> Thermal/int340x: prevent bounds-check bypass via speculative execution
> ipv6: prevent bounds-check bypass via speculative execution
> ipv4: prevent bounds-check bypass via speculative execution
> vfs, fdtable: prevent bounds-check bypass via speculative execution
> net: mpls: prevent bounds-check bypass via speculative execution
> udf: prevent bounds-check bypass via speculative execution
> userns: prevent bounds-check bypass via speculative execution
>
> Mark Rutland (4):
> asm-generic/barrier: add generic nospec helpers
> Documentation: document nospec helpers
> arm64: implement nospec_ptr()
> arm: implement nospec_ptr()
So considering the recent publication of [1], how come we all of a sudden
don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
Is this going to be handled in eBPF in some other way?
Without that in place, and considering Jann Horn's paper, it would seem
like PTI doesn't really lock it down fully, right?
[1] https://bugs.chromium.org/p/project-zero/issues/attachmentText?aid=287305
--
Jiri Kosina
SUSE Labs
Static analysis reports that 'queue' may be a user controlled value that
is used as a data dependency to read 'txq_params' from the
'priv->tx_queue_params.params' 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 value of 'txq_params'.
In this case 'txq_params' is referenced later in the function.
Based on an original patch by Elena Reshetova.
Cc: Solomon Peachy <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Elena Reshetova <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/net/wireless/st/cw1200/sta.c | 10 ++++++----
drivers/net/wireless/st/cw1200/wsm.h | 4 +---
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/sta.c b/drivers/net/wireless/st/cw1200/sta.c
index 38678e9a0562..886942617f14 100644
--- a/drivers/net/wireless/st/cw1200/sta.c
+++ b/drivers/net/wireless/st/cw1200/sta.c
@@ -14,6 +14,7 @@
#include <linux/firmware.h>
#include <linux/module.h>
#include <linux/etherdevice.h>
+#include <linux/compiler.h>
#include "cw1200.h"
#include "sta.h"
@@ -612,18 +613,19 @@ int cw1200_conf_tx(struct ieee80211_hw *dev, struct ieee80211_vif *vif,
u16 queue, const struct ieee80211_tx_queue_params *params)
{
struct cw1200_common *priv = dev->priv;
+ struct wsm_set_tx_queue_params *txq_params;
int ret = 0;
/* To prevent re-applying PM request OID again and again*/
bool old_uapsd_flags;
mutex_lock(&priv->conf_mutex);
- if (queue < dev->queues) {
+ if ((txq_params = nospec_array_ptr(priv->tx_queue_params.params,
+ queue, dev->queues))) {
old_uapsd_flags = le16_to_cpu(priv->uapsd_info.uapsd_flags);
- WSM_TX_QUEUE_SET(&priv->tx_queue_params, queue, 0, 0, 0);
- ret = wsm_set_tx_queue_params(priv,
- &priv->tx_queue_params.params[queue], queue);
+ WSM_TX_QUEUE_SET(txq_params, 0, 0, 0);
+ ret = wsm_set_tx_queue_params(priv, txq_params, queue);
if (ret) {
ret = -EINVAL;
goto out;
diff --git a/drivers/net/wireless/st/cw1200/wsm.h b/drivers/net/wireless/st/cw1200/wsm.h
index 48086e849515..8c8d9191e233 100644
--- a/drivers/net/wireless/st/cw1200/wsm.h
+++ b/drivers/net/wireless/st/cw1200/wsm.h
@@ -1099,10 +1099,8 @@ struct wsm_tx_queue_params {
};
-#define WSM_TX_QUEUE_SET(queue_params, queue, ack_policy, allowed_time,\
- max_life_time) \
+#define WSM_TX_QUEUE_SET(p, ack_policy, allowed_time, max_life_time) \
do { \
- struct wsm_set_tx_queue_params *p = &(queue_params)->params[queue]; \
p->ackPolicy = (ack_policy); \
p->allowedMediumTime = (allowed_time); \
p->maxTransmitLifetime = (max_life_time); \
On 01/05/18 22:30, Dan Williams wrote:
> On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
>> Please expand this.
>>
>> It is not clear what the static analysis is looking for. Have a clear
>> description of what is being fixed is crucial for allowing any of these
>> changes.
>>
>> For the details given in the change description what I read is magic
>> changes because a magic process says this code is vulnerable.
>
> Yes, that was my first reaction to the patches as well, I try below to
> add some more background and guidance, but in the end these are static
> analysis reports across a wide swath of sub-systems. It's going to
> take some iteration with domain experts to improve the patch
> descriptions, and that's the point of this series, to get the better
> trained eyes from the actual sub-system owners to take a look at these
> reports.
More information about what the static analysis is looking for would
definitely be welcome.
Additionally, since the analysis tool is not publicly available, how are
authors of new kernel code assumed to verify whether or not their code
needs to use nospec_array_ptr()? How are reviewers of kernel code
assumed to verify whether or not nospec_array_ptr() is missing where it
should be used?
Since this patch series only modifies the upstream kernel, how will
out-of-tree drivers be fixed, e.g. the nVidia driver and the Android
drivers?
Thanks,
Bart.
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 <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Elena Reshetova <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> 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>
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 'priv->qos_params' 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 'priv->qos_params[queue]'.
>
> Based on an original patch by Elena Reshetova.
>
> Cc: Christian Lamparter <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Elena Reshetova <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> drivers/net/wireless/intersil/p54/main.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index ab6d39e12069..85c9cbee35fc 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
[...]
> @@ -411,12 +412,13 @@ static int p54_conf_tx(struct ieee80211_hw *dev,
> const struct ieee80211_tx_queue_params *params)
> {
> struct p54_common *priv = dev->priv;
> + struct p54_edcf_queue_param *p54_q;
> int ret;
>
> mutex_lock(&priv->conf_mutex);
> - if (queue < dev->queues) {
> - P54_SET_QUEUE(priv->qos_params[queue], params->aifs,
> - params->cw_min, params->cw_max, params->txop);
> + if ((p54_q = nospec_array_ptr(priv->qos_params, queue, dev->queues))) {
Same complaint here...
> + P54_SET_QUEUE(p54_q[0], params->aifs, params->cw_min,
> + params->cw_max, params->txop);
> ret = p54_set_edcf(priv);
> } else
> ret = -EINVAL;
>
MBR, Sergei
On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
> Dan Williams <[email protected]> writes:
>
>> 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 [1]
>> and the Documentation patch in this series."
>>
>> This series incorporates Mark Rutland's latest api and adds the x86
>> specific implementation of nospec_barrier. The
>> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
>> wide analysis performed by Elena Reshetova to address static analysis
>> reports where speculative execution on a userspace controlled value
>> could bypass a bounds check. The patches address a precondition for the
>> attack discussed in the Spectre paper [2].
>
> Please expand this.
>
> It is not clear what the static analysis is looking for. Have a clear
> description of what is being fixed is crucial for allowing any of these
> changes.
>
> For the details given in the change description what I read is magic
> changes because a magic process says this code is vunlerable.
Yes, that was my first reaction to the patches as well, I try below to
add some more background and guidance, but in the end these are static
analysis reports across a wide swath of sub-systems. It's going to
take some iteration with domain experts to improve the patch
descriptions, and that's the point of this series, to get the better
trained eyes from the actual sub-system owners to take a look at these
reports.
For example, I'm looking for feedback like what Srinivas gave where he
identified that the report is bogus, the branch condition can not be
seeded with bad values in that path. Be like Srinivas.
> Given the similarities in the code that is being patched to many other
> places in the kernel it is not at all clear that this small set of
> changes is sufficient for any purpose.
I find this assertion absurd, when in the past have we as kernel
developers ever been handed a static analysis report and then
questioned why the static analysis did not flag other call sites
before first reviewing the ones it did find?
>> A consideration worth noting for reviewing these patches is to weigh the
>> dramatic cost of being wrong about whether a given report is exploitable
>> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
>> lets make the bar for applying these patches be "can you prove that the
>> bounds check bypass is *not* exploitable". Consider that the Spectre
>> paper reports one example of a speculation window being ~180 cycles.
>
>
>> Note that there is also a proposal from Linus, array_access [3], that
>> attempts to quash speculative execution past a bounds check without
>> introducing an lfence instruction. That may be a future optimization
>> possibility that is compatible with this api, but it would appear to
>> need guarantees from the compiler that it is not clear the kernel can
>> rely on at this point. It is also not clear that it would be a
>> significant performance win vs lfence.
>
> It is also not clear that these changes fix anything, or are in any
> sense correct for the problem they are trying to fix as the problem
> is not clearly described.
I'll try my best. I don't have first hand knowledge of how the static
analyzer is doing this job, and I don't think it matters for
evaluating these reports. I'll give you my thoughts on how I would
handle one of these reports if it flagged one of the sub-systems I
maintain.
Start with the example from the Spectre paper:
if (x < array1_size)
y = array2[array1[x] * 256];
In all the patches 'x' and 'array1' are called out explicitly. For example:
net: mpls: prevent bounds-check bypass via speculative execution
Static analysis reports that 'index' may be a user controlled value that
is used as a data dependency reading 'rt' from the 'platform_label'
array...
So the first thing to review is whether the analyzer got it wrong and
'x' is not arbitrarily controllable by userspace to cause speculation
outside of the checked bounds. Be like Srinivas. The next step is to
ask whether the code can be refactored so that 'x' is sanitized
earlier in the call stack, especially if the nospec_array_ptr() lands
in a hot path. The next aspect that I expect most would be tempted to
go check is whether 'array2[array1[x]]' occurs later in the code
stream, but with speculation windows being architecture dependent and
potentially large (~180 cycles in one case says the paper) I submit
that we should err on the side of caution and not guess if that second
dependent read has been emitted somewhere in the instruction stream.
> In at least one place (mpls) you are patching a fast path. Compile out
> or don't load mpls by all means. But it is not acceptable to change the
> fast path without even considering performance.
Performance matters greatly, but I need help to identify a workload
that is representative for this fast path to see what, if any, impact
is incurred. Even better is a review that says "nope, 'index' is not
subject to arbitrary userspace control at this point, drop the patch."
On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
> On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <[email protected]> wrote:
> > On Fri, 5 Jan 2018, Dan Williams wrote:
> >
> > [ ... snip ... ]
> >> Andi Kleen (1):
> >> x86, barrier: stop speculation for failed access_ok
> >>
> >> Dan Williams (13):
> >> x86: implement nospec_barrier()
> >> [media] uvcvideo: prevent bounds-check bypass via speculative execution
> >> carl9170: prevent bounds-check bypass via speculative execution
> >> p54: prevent bounds-check bypass via speculative execution
> >> qla2xxx: prevent bounds-check bypass via speculative execution
> >> cw1200: prevent bounds-check bypass via speculative execution
> >> Thermal/int340x: prevent bounds-check bypass via speculative execution
> >> ipv6: prevent bounds-check bypass via speculative execution
> >> ipv4: prevent bounds-check bypass via speculative execution
> >> vfs, fdtable: prevent bounds-check bypass via speculative execution
> >> net: mpls: prevent bounds-check bypass via speculative execution
> >> udf: prevent bounds-check bypass via speculative execution
> >> userns: prevent bounds-check bypass via speculative execution
> >>
> >> Mark Rutland (4):
> >> asm-generic/barrier: add generic nospec helpers
> >> Documentation: document nospec helpers
> >> arm64: implement nospec_ptr()
> >> arm: implement nospec_ptr()
> >
> > So considering the recent publication of [1], how come we all of a sudden
> > don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
> > LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
> >
> > Is this going to be handled in eBPF in some other way?
> >
> > Without that in place, and considering Jann Horn's paper, it would seem
> > like PTI doesn't really lock it down fully, right?
>
> Here is the latest (v3) bpf fix:
>
> https://patchwork.ozlabs.org/patch/856645/
>
> I currently have v2 on my 'nospec' branch and will move that to v3 for
> the next update, unless it goes upstream before then.
That patch seems specific to CONFIG_BPF_SYSCALL. Is the bpf() syscall
the only attack vector? Or are there other ways to run bpf programs
that we should be worried about?
--
Josh
On Thu, Jan 11, 2018 at 1:54 AM, Jiri Kosina <[email protected]> wrote:
> On Tue, 9 Jan 2018, Josh Poimboeuf wrote:
>
>> On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
>> > On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <[email protected]> wrote:
>> > > On Fri, 5 Jan 2018, Dan Williams wrote:
>> > >
>> > > [ ... snip ... ]
>> > >> Andi Kleen (1):
>> > >> x86, barrier: stop speculation for failed access_ok
>> > >>
>> > >> Dan Williams (13):
>> > >> x86: implement nospec_barrier()
>> > >> [media] uvcvideo: prevent bounds-check bypass via speculative execution
>> > >> carl9170: prevent bounds-check bypass via speculative execution
>> > >> p54: prevent bounds-check bypass via speculative execution
>> > >> qla2xxx: prevent bounds-check bypass via speculative execution
>> > >> cw1200: prevent bounds-check bypass via speculative execution
>> > >> Thermal/int340x: prevent bounds-check bypass via speculative execution
>> > >> ipv6: prevent bounds-check bypass via speculative execution
>> > >> ipv4: prevent bounds-check bypass via speculative execution
>> > >> vfs, fdtable: prevent bounds-check bypass via speculative execution
>> > >> net: mpls: prevent bounds-check bypass via speculative execution
>> > >> udf: prevent bounds-check bypass via speculative execution
>> > >> userns: prevent bounds-check bypass via speculative execution
>> > >>
>> > >> Mark Rutland (4):
>> > >> asm-generic/barrier: add generic nospec helpers
>> > >> Documentation: document nospec helpers
>> > >> arm64: implement nospec_ptr()
>> > >> arm: implement nospec_ptr()
>> > >
>> > > So considering the recent publication of [1], how come we all of a sudden
>> > > don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
>> > > LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
>> > >
>> > > Is this going to be handled in eBPF in some other way?
>> > >
>> > > Without that in place, and considering Jann Horn's paper, it would seem
>> > > like PTI doesn't really lock it down fully, right?
>> >
>> > Here is the latest (v3) bpf fix:
>> >
>> > https://patchwork.ozlabs.org/patch/856645/
>> >
>> > I currently have v2 on my 'nospec' branch and will move that to v3 for
>> > the next update, unless it goes upstream before then.
>
> Daniel, I guess you're planning to send this still for 4.15?
It's pending in the bpf.git tree:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=b2157399cc9
>> That patch seems specific to CONFIG_BPF_SYSCALL. Is the bpf() syscall
>> the only attack vector? Or are there other ways to run bpf programs
>> that we should be worried about?
>
> Seems like Alexei is probably the only person in the whole universe who
> isn't CCed here ... let's fix that.
He will be cc'd on v2 of this series which will be available later today.
On Mon, 8 Jan 2018 11:08:36 +0100
Peter Zijlstra <[email protected]> wrote:
> On Fri, Jan 05, 2018 at 10:30:16PM -0800, Dan Williams wrote:
> > On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <[email protected]> wrote:
> > > In at least one place (mpls) you are patching a fast path. Compile out
> > > or don't load mpls by all means. But it is not acceptable to change the
> > > fast path without even considering performance.
> >
> > Performance matters greatly, but I need help to identify a workload
> > that is representative for this fast path to see what, if any, impact
> > is incurred. Even better is a review that says "nope, 'index' is not
> > subject to arbitrary userspace control at this point, drop the patch."
>
> I think we're focussing a little too much on pure userspace. That is, we
> should be saying under the attackers control. Inbound network packets
> could equally be under the attackers control.
Inbound network packets don't come with a facility to read back and do
cache timimg. For the more general case, timing attacks on network
activity are not exactly new, and you have to mitigate them in user space
because most of them are about how many instructions you execute on each
path. The ancient classic being telling if a user exists by seeing if the
password was actually checked.
Alan
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 <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Elena Reshetova <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> 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 Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <[email protected]> wrote:
> On Fri, 5 Jan 2018, Dan Williams wrote:
>
> [ ... snip ... ]
>> Andi Kleen (1):
>> x86, barrier: stop speculation for failed access_ok
>>
>> Dan Williams (13):
>> x86: implement nospec_barrier()
>> [media] uvcvideo: prevent bounds-check bypass via speculative execution
>> carl9170: prevent bounds-check bypass via speculative execution
>> p54: prevent bounds-check bypass via speculative execution
>> qla2xxx: prevent bounds-check bypass via speculative execution
>> cw1200: prevent bounds-check bypass via speculative execution
>> Thermal/int340x: prevent bounds-check bypass via speculative execution
>> ipv6: prevent bounds-check bypass via speculative execution
>> ipv4: prevent bounds-check bypass via speculative execution
>> vfs, fdtable: prevent bounds-check bypass via speculative execution
>> net: mpls: prevent bounds-check bypass via speculative execution
>> udf: prevent bounds-check bypass via speculative execution
>> userns: prevent bounds-check bypass via speculative execution
>>
>> Mark Rutland (4):
>> asm-generic/barrier: add generic nospec helpers
>> Documentation: document nospec helpers
>> arm64: implement nospec_ptr()
>> arm: implement nospec_ptr()
>
> So considering the recent publication of [1], how come we all of a sudden
> don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
> LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
>
> Is this going to be handled in eBPF in some other way?
>
> Without that in place, and considering Jann Horn's paper, it would seem
> like PTI doesn't really lock it down fully, right?
Here is the latest (v3) bpf fix:
https://patchwork.ozlabs.org/patch/856645/
I currently have v2 on my 'nospec' branch and will move that to v3 for
the next update, unless it goes upstream before then.
On 01/11/2018 04:58 PM, Dan Williams wrote:
> On Thu, Jan 11, 2018 at 1:54 AM, Jiri Kosina <[email protected]> wrote:
>> On Tue, 9 Jan 2018, Josh Poimboeuf wrote:
>>> On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
>>>> On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <[email protected]> wrote:
>>>>> On Fri, 5 Jan 2018, Dan Williams wrote:
>>>>>
>>>>> [ ... snip ... ]
>>>>>> Andi Kleen (1):
>>>>>> x86, barrier: stop speculation for failed access_ok
>>>>>>
>>>>>> Dan Williams (13):
>>>>>> x86: implement nospec_barrier()
>>>>>> [media] uvcvideo: prevent bounds-check bypass via speculative execution
>>>>>> carl9170: prevent bounds-check bypass via speculative execution
>>>>>> p54: prevent bounds-check bypass via speculative execution
>>>>>> qla2xxx: prevent bounds-check bypass via speculative execution
>>>>>> cw1200: prevent bounds-check bypass via speculative execution
>>>>>> Thermal/int340x: prevent bounds-check bypass via speculative execution
>>>>>> ipv6: prevent bounds-check bypass via speculative execution
>>>>>> ipv4: prevent bounds-check bypass via speculative execution
>>>>>> vfs, fdtable: prevent bounds-check bypass via speculative execution
>>>>>> net: mpls: prevent bounds-check bypass via speculative execution
>>>>>> udf: prevent bounds-check bypass via speculative execution
>>>>>> userns: prevent bounds-check bypass via speculative execution
>>>>>>
>>>>>> Mark Rutland (4):
>>>>>> asm-generic/barrier: add generic nospec helpers
>>>>>> Documentation: document nospec helpers
>>>>>> arm64: implement nospec_ptr()
>>>>>> arm: implement nospec_ptr()
>>>>>
>>>>> So considering the recent publication of [1], how come we all of a sudden
>>>>> don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
>>>>> LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
>>>>>
>>>>> Is this going to be handled in eBPF in some other way?
>>>>>
>>>>> Without that in place, and considering Jann Horn's paper, it would seem
>>>>> like PTI doesn't really lock it down fully, right?
>>>>
>>>> Here is the latest (v3) bpf fix:
>>>>
>>>> https://patchwork.ozlabs.org/patch/856645/
>>>>
>>>> I currently have v2 on my 'nospec' branch and will move that to v3 for
>>>> the next update, unless it goes upstream before then.
>>
>> Daniel, I guess you're planning to send this still for 4.15?
>
> It's pending in the bpf.git tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=b2157399cc9
Sorry for the delay, just noticed the question now since not on Cc either:
It made it into in DaveM's tree already and part of his latest pull-req
to Linus.
>>> That patch seems specific to CONFIG_BPF_SYSCALL. Is the bpf() syscall
>>> the only attack vector? Or are there other ways to run bpf programs
>>> that we should be worried about?
>>
>> Seems like Alexei is probably the only person in the whole universe who
>> isn't CCed here ... let's fix that.
>
> He will be cc'd on v2 of this series which will be available later today.