[PATCH 00/18] prevent bounds-check bypass via speculative execution

[PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

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

[PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[Dan Williams]

Static analysis reports that 'handle' may be a user controlled value
that is used as a data dependency to read 'sp' from the
'req->outstanding_cmds' 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 'sp'. In this
case 'sp' is directly dereferenced later in the function.

Based on an original patch by Elena Reshetova.

Cc: qla2xxx-upstream@qlogic.com
Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/qla2xxx/qla_mr.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index d5da3981cefe..128b41de3784 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -9,6 +9,7 @@
 #include <linux/ktime.h>
 #include <linux/pci.h>
 #include <linux/ratelimit.h>
+#include <linux/compiler.h>
 #include <linux/vmalloc.h>
 #include <linux/bsg-lib.h>
 #include <scsi/scsi_tcq.h>
@@ -2275,7 +2276,7 @@ qlafx00_ioctl_iosb_entry(scsi_qla_host_t *vha, struct req_que *req,
 static void
 qlafx00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 {
-	srb_t		*sp;
+	srb_t		*sp, **elem;
 	fc_port_t	*fcport;
 	struct scsi_cmnd *cp;
 	struct sts_entry_fx00 *sts;
@@ -2304,8 +2305,9 @@ qlafx00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 	req = ha->req_q_map[que];
 
 	/* Validate handle. */
-	if (handle < req->num_outstanding_cmds)
-		sp = req->outstanding_cmds[handle];
+	if ((elem = nospec_array_ptr(req->outstanding_cmds, handle,
+					req->num_outstanding_cmds)))
+		sp = *elem;
 	else
 		sp = NULL;
 
@@ -2626,7 +2628,7 @@ static void
 qlafx00_multistatus_entry(struct scsi_qla_host *vha,
 	struct rsp_que *rsp, void *pkt)
 {
-	srb_t		*sp;
+	srb_t		*sp, **elem;
 	struct multi_sts_entry_fx00 *stsmfx;
 	struct qla_hw_data *ha = vha->hw;
 	uint32_t handle, hindex, handle_count, i;
@@ -2655,8 +2657,9 @@ qlafx00_multistatus_entry(struct scsi_qla_host *vha,
 		req = ha->req_q_map[que];
 
 		/* Validate handle. */
-		if (handle < req->num_outstanding_cmds)
-			sp = req->outstanding_cmds[handle];
+		if ((elem = nospec_array_ptr(req->outstanding_cmds, handle,
+						req->num_outstanding_cmds)))
+			sp = *elem;
 		else
 			sp = NULL;
 

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Eric W. Biederman]

Dan Williams <dan.j.williams@intel.com> 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" <ebiederm@xmission.com>

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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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."

Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[Greg KH]

On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> Static analysis reports that 'handle' may be a user controlled value
> that is used as a data dependency to read 'sp' from the
> 'req->outstanding_cmds' 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 'sp'. In this
> case 'sp' is directly dereferenced later in the function.

I'm pretty sure that 'handle' comes from the hardware, not from
userspace, from what I can tell here.  If we want to start auditing
__iomem data sources, great!  But that's a bigger task, and one I don't
think we are ready to tackle...

thanks,

greg k-h

Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[Greg KH]

On Sat, Jan 06, 2018 at 10:03:22AM +0100, Greg KH wrote:
> On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> > Static analysis reports that 'handle' may be a user controlled value
> > that is used as a data dependency to read 'sp' from the
> > 'req->outstanding_cmds' 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 'sp'. In this
> > case 'sp' is directly dereferenced later in the function.
> 
> I'm pretty sure that 'handle' comes from the hardware, not from
> userspace, from what I can tell here.  If we want to start auditing
> __iomem data sources, great!  But that's a bigger task, and one I don't
> think we are ready to tackle...

And as Peter Zijlstra has already mentioned, if we have to look at those
codepaths, USB drivers are the first up for that mess, so having access
to the coverity rules would be a great help in starting that effort.

thanks,

greg k-h

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Florian Fainelli]

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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Arjan van de Ven]

> 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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <dan.j.williams@intel.com> 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.

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 11:37 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <dan.j.williams@intel.com> 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/

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Peter Zijlstra]

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 <ebiederm@xmission.com> 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.

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Alan Cox]

On Mon, 8 Jan 2018 11:08:36 +0100
Peter Zijlstra <peterz@infradead.org> 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 <ebiederm@xmission.com> 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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Peter Zijlstra]

On Mon, Jan 08, 2018 at 11:43:42AM +0000, Alan Cox wrote:
> On Mon, 8 Jan 2018 11:08:36 +0100
> Peter Zijlstra <peterz@infradead.org> 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 <ebiederm@xmission.com> 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?

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Bart Van Assche]

On 01/05/18 22:30, Dan Williams wrote:
> On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <ebiederm@xmission.com> 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.

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Ingo Molnar]

* Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:

> On Mon, 8 Jan 2018 11:08:36 +0100
> Peter Zijlstra <peterz@infradead.org> 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 <ebiederm@xmission.com> 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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Jiri Kosina]

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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <jikos@kernel.org> 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.

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Josh Poimboeuf]

On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
> On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina <jikos@kernel.org> 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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Jiri Kosina]

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 <jikos@kernel.org> 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

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Dan Williams]

On Thu, Jan 11, 2018 at 1:54 AM, Jiri Kosina <jikos@kernel.org> 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 <jikos@kernel.org> 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.

Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Daniel Borkmann]

On 01/11/2018 04:58 PM, Dan Williams wrote:
> On Thu, Jan 11, 2018 at 1:54 AM, Jiri Kosina <jikos@kernel.org> 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 <jikos@kernel.org> 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.

Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
>> Static analysis reports that 'handle' may be a user controlled value
>> that is used as a data dependency to read 'sp' from the
>> 'req->outstanding_cmds' 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 'sp'. In this
>> case 'sp' is directly dereferenced later in the function.
>
> I'm pretty sure that 'handle' comes from the hardware, not from
> userspace, from what I can tell here.  If we want to start auditing
> __iomem data sources, great!  But that's a bigger task, and one I don't
> think we are ready to tackle...

I think it falls in the hygiene bucket of shutting off an array index
from a source that could be under attacker control. Should we leave
this one un-patched while we decide if we generally have a problem
with trusting completion 'tags' from hardware? My vote is patch it for
now.

Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[Greg KH]

On Thu, Jan 11, 2018 at 02:15:12PM -0800, Dan Williams wrote:
> On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> >> Static analysis reports that 'handle' may be a user controlled value
> >> that is used as a data dependency to read 'sp' from the
> >> 'req->outstanding_cmds' 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 'sp'. In this
> >> case 'sp' is directly dereferenced later in the function.
> >
> > I'm pretty sure that 'handle' comes from the hardware, not from
> > userspace, from what I can tell here.  If we want to start auditing
> > __iomem data sources, great!  But that's a bigger task, and one I don't
> > think we are ready to tackle...
> 
> I think it falls in the hygiene bucket of shutting off an array index
> from a source that could be under attacker control. Should we leave
> this one un-patched while we decide if we generally have a problem
> with trusting completion 'tags' from hardware? My vote is patch it for
> now.

Hah, if you are worried about "tags" from hardware, we have a lot more
auditing to do, right?  I don't think anyone has looked into just basic
"bounds checking" for that type of information.  For USB devices we have
_just_ started doing that over the past year, the odds of anyone looking
at PCI devices for this same problem is slim-to-none.

Again, here are my questions/objections right now to this series:
	- How can we audit this stuff?
	- How did you audit this stuff to find these usages?
	- How do you know that this series fixes all of the issues?
	- What exact tree/date did you run your audit against?
	- How do you know that linux-next does not contain a boatload
	  more problems that we need to go back and fix after 4.16-rc1
	  is out?
	- How can we prevent this type of pattern showing up again?
	- How can we audit the data coming from hardware correctly?

I'm all for merging this series, but if anyone things that somehow the
whole problem is now "solved" in this area, they are sorely mistaken.

thanks,

greg k-h

Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution

[James Bottomley]

On Fri, 2018-01-12 at 08:27 +0100, Greg KH wrote:
> On Thu, Jan 11, 2018 at 02:15:12PM -0800, Dan Williams wrote:
> > 
> > On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org
> > > wrote:
> > > 
> > > On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> > > > 
> > > > Static analysis reports that 'handle' may be a user controlled
> > > > value that is used as a data dependency to read 'sp' from the
> > > > 'req->outstanding_cmds' 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 'sp'. In this case 'sp' is directly
> > > > dereferenced later in the function.
> > > 
> > > I'm pretty sure that 'handle' comes from the hardware, not from
> > > userspace, from what I can tell here.  If we want to start
> > > auditing __iomem data sources, great!  But that's a bigger task,
> > > and one I don't think we are ready to tackle...
> > 
> > I think it falls in the hygiene bucket of shutting off an array
> > index from a source that could be under attacker control. Should we
> > leave this one un-patched while we decide if we generally have a
> > problem with trusting completion 'tags' from hardware? My vote is
> > patch it for now.
> 
> Hah, if you are worried about "tags" from hardware, we have a lot
> more auditing to do, right? 

We'd also have a lot more to do: the assumption would have to be
malicious hardware and most hardware has access to fairly vital stuff
directly.  I really don't think we have to worry about side channel
attacks from hardware until the direct attack vector is closed.

James