[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 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Dan Williams]

Static analysis reports that 'index' may be a user controlled value that
is used as a data dependency to read 'pin' from the
'selector->baSourceID' 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 'pin'.

Based on an original patch by Elena Reshetova.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 3e7e283a44a8..7442626dc20e 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -22,6 +22,7 @@
 #include <linux/mm.h>
 #include <linux/wait.h>
 #include <linux/atomic.h>
+#include <linux/compiler.h>
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
@@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
 	struct uvc_entity *iterm = NULL;
 	u32 index = input->index;
 	int pin = 0;
+	__u8 *elem;
 
 	if (selector == NULL ||
 	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
@@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
 				break;
 		}
 		pin = iterm->id;
-	} else if (index < selector->bNrInPins) {
-		pin = selector->baSourceID[index];
+	} else if ((elem = nospec_array_ptr(selector->baSourceID, index,
+					selector->bNrInPins))) {
+		pin = *elem;
 		list_for_each_entry(iterm, &chain->entities, chain) {
 			if (!UVC_ENTITY_IS_ITERM(iterm))
 				continue;

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

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 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> Static analysis reports that 'index' may be a user controlled value that
> is used as a data dependency to read 'pin' from the
> 'selector->baSourceID' 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 'pin'.
> 
> Based on an original patch by Elena Reshetova.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 3e7e283a44a8..7442626dc20e 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -22,6 +22,7 @@
>  #include <linux/mm.h>
>  #include <linux/wait.h>
>  #include <linux/atomic.h>
> +#include <linux/compiler.h>
>  
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>  	struct uvc_entity *iterm = NULL;
>  	u32 index = input->index;
>  	int pin = 0;
> +	__u8 *elem;
>  
>  	if (selector == NULL ||
>  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>  				break;
>  		}
>  		pin = iterm->id;
> -	} else if (index < selector->bNrInPins) {
> -		pin = selector->baSourceID[index];
> +	} else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> +					selector->bNrInPins))) {
> +		pin = *elem;

I dug through this before, and I couldn't find where index came from
userspace, I think seeing the coverity rule would be nice.

And if this value really is user controlled, then why is this the only
v4l driver that is affected?  This is a common callback.

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> > Static analysis reports that 'index' may be a user controlled value that
> > is used as a data dependency to read 'pin' from the
> > 'selector->baSourceID' 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 'pin'.
> > 
> > Based on an original patch by Elena Reshetova.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 3e7e283a44a8..7442626dc20e 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/wait.h>
> >  #include <linux/atomic.h>
> > +#include <linux/compiler.h>
> >  
> >  #include <media/v4l2-common.h>
> >  #include <media/v4l2-ctrls.h>
> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> >  	struct uvc_entity *iterm = NULL;
> >  	u32 index = input->index;
> >  	int pin = 0;
> > +	__u8 *elem;
> >  
> >  	if (selector == NULL ||
> >  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> >  				break;
> >  		}
> >  		pin = iterm->id;
> > -	} else if (index < selector->bNrInPins) {
> > -		pin = selector->baSourceID[index];
> > +	} else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> > +					selector->bNrInPins))) {
> > +		pin = *elem;
> 
> I dug through this before, and I couldn't find where index came from
> userspace, I think seeing the coverity rule would be nice.

Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
crazy complex (rightfully so), it's amazing that coverity could navigate
that whole thing :)

While I'm all for fixing this type of thing, I feel like we need to do
something "else" for this as playing whack-a-mole for this pattern is
going to be a never-ending battle for all drivers for forever.  Either
we need some way to mark this data path to make it easy for tools like
sparse to flag easily, or we need to catch the issue in the driver
subsystems, which unfortunatly, would harm the drivers that don't have
this type of issue (like here.)

I'm guessing that other operating systems, which don't have the luxury
of auditing all of their drivers are going for the "big hammer in the
subsystem" type of fix, right?

I don't have a good answer for this, but if there was some better way to
rewrite these types of patterns to just prevent the need for the
nospec_array_ptr() type thing, that might be the best overall for
everyone.  Much like ebpf did with their changes.  That way a simple
coccinelle rule would be able to catch the pattern and rewrite it.

Or am I just dreaming?

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 1:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
>> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
>> > Static analysis reports that 'index' may be a user controlled value that
>> > is used as a data dependency to read 'pin' from the
>> > 'selector->baSourceID' 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 'pin'.
>> >
>> > Based on an original patch by Elena Reshetova.
>> >
>> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> > Cc: linux-media@vger.kernel.org
>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> >  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>> > index 3e7e283a44a8..7442626dc20e 100644
>> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> > @@ -22,6 +22,7 @@
>> >  #include <linux/mm.h>
>> >  #include <linux/wait.h>
>> >  #include <linux/atomic.h>
>> > +#include <linux/compiler.h>
>> >
>> >  #include <media/v4l2-common.h>
>> >  #include <media/v4l2-ctrls.h>
>> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>> >     struct uvc_entity *iterm = NULL;
>> >     u32 index = input->index;
>> >     int pin = 0;
>> > +   __u8 *elem;
>> >
>> >     if (selector == NULL ||
>> >         (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
>> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>> >                             break;
>> >             }
>> >             pin = iterm->id;
>> > -   } else if (index < selector->bNrInPins) {
>> > -           pin = selector->baSourceID[index];
>> > +   } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
>> > +                                   selector->bNrInPins))) {
>> > +           pin = *elem;
>>
>> I dug through this before, and I couldn't find where index came from
>> userspace, I think seeing the coverity rule would be nice.
>
> Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
> crazy complex (rightfully so), it's amazing that coverity could navigate
> that whole thing :)
>
> While I'm all for fixing this type of thing, I feel like we need to do
> something "else" for this as playing whack-a-mole for this pattern is
> going to be a never-ending battle for all drivers for forever.  Either
> we need some way to mark this data path to make it easy for tools like
> sparse to flag easily, or we need to catch the issue in the driver
> subsystems, which unfortunatly, would harm the drivers that don't have
> this type of issue (like here.)
>
> I'm guessing that other operating systems, which don't have the luxury
> of auditing all of their drivers are going for the "big hammer in the
> subsystem" type of fix, right?
>
> I don't have a good answer for this, but if there was some better way to
> rewrite these types of patterns to just prevent the need for the
> nospec_array_ptr() type thing, that might be the best overall for
> everyone.  Much like ebpf did with their changes.  That way a simple
> coccinelle rule would be able to catch the pattern and rewrite it.
>
> Or am I just dreaming?

At least on the coccinelle front you're dreaming. Julia already took a
look and said:

"I don't think Coccinelle would be good for doing this (ie
implementing taint analysis) because the dataflow is too complicated."

Perhaps the Coverity instance Dave mentioned at Ksummit 2012 has a
role to play here?

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 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Sat, Jan 06, 2018 at 09:41:17AM -0800, Dan Williams wrote:
> On Sat, Jan 6, 2018 at 1:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> >> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> >> > Static analysis reports that 'index' may be a user controlled value that
> >> > is used as a data dependency to read 'pin' from the
> >> > 'selector->baSourceID' 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 'pin'.
> >> >
> >> > Based on an original patch by Elena Reshetova.
> >> >
> >> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> > Cc: linux-media@vger.kernel.org
> >> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> >> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> > ---
> >> >  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >> > index 3e7e283a44a8..7442626dc20e 100644
> >> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> > @@ -22,6 +22,7 @@
> >> >  #include <linux/mm.h>
> >> >  #include <linux/wait.h>
> >> >  #include <linux/atomic.h>
> >> > +#include <linux/compiler.h>
> >> >
> >> >  #include <media/v4l2-common.h>
> >> >  #include <media/v4l2-ctrls.h>
> >> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> >> >     struct uvc_entity *iterm = NULL;
> >> >     u32 index = input->index;
> >> >     int pin = 0;
> >> > +   __u8 *elem;
> >> >
> >> >     if (selector == NULL ||
> >> >         (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> >> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> >> >                             break;
> >> >             }
> >> >             pin = iterm->id;
> >> > -   } else if (index < selector->bNrInPins) {
> >> > -           pin = selector->baSourceID[index];
> >> > +   } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> >> > +                                   selector->bNrInPins))) {
> >> > +           pin = *elem;
> >>
> >> I dug through this before, and I couldn't find where index came from
> >> userspace, I think seeing the coverity rule would be nice.
> >
> > Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
> > crazy complex (rightfully so), it's amazing that coverity could navigate
> > that whole thing :)
> >
> > While I'm all for fixing this type of thing, I feel like we need to do
> > something "else" for this as playing whack-a-mole for this pattern is
> > going to be a never-ending battle for all drivers for forever.  Either
> > we need some way to mark this data path to make it easy for tools like
> > sparse to flag easily, or we need to catch the issue in the driver
> > subsystems, which unfortunatly, would harm the drivers that don't have
> > this type of issue (like here.)
> >
> > I'm guessing that other operating systems, which don't have the luxury
> > of auditing all of their drivers are going for the "big hammer in the
> > subsystem" type of fix, right?
> >
> > I don't have a good answer for this, but if there was some better way to
> > rewrite these types of patterns to just prevent the need for the
> > nospec_array_ptr() type thing, that might be the best overall for
> > everyone.  Much like ebpf did with their changes.  That way a simple
> > coccinelle rule would be able to catch the pattern and rewrite it.
> >
> > Or am I just dreaming?
> 
> At least on the coccinelle front you're dreaming. Julia already took a
> look and said:
> 
> "I don't think Coccinelle would be good for doing this (ie
> implementing taint analysis) because the dataflow is too complicated."

Sorry for the confusion, no, I don't mean the "taint tracking", I mean
the generic pattern of "speculative out of bounds access" that we are
fixing here.

Yes, as you mentioned before, there are tons of false-positives in the
tree, as to find the real problems you have to show that userspace
controls the access index.  But if we have a generic pattern that can
rewrite that type of logic into one where it does not matter at all
(i.e. like the ebpf proposed changes), then it would not be an issue if
they are false or not, we just rewrite them all to be safe.

We need to find some way not only to fix these issues now (like you are
doing with this series), but to prevent them from every coming back into
the codebase again.  It's that second part that we need to keep in the
back of our minds here, while doing the first portion of this work.

> Perhaps the Coverity instance Dave mentioned at Ksummit 2012 has a
> role to play here?

We have a coverity instance that all kernel developers have access to
(just sign up and we grant it.)  We have at least one person working
full time on fixing up errors that this instance reports.  So if we
could get those rules added (which is why I asked for them), it would be
a great first line of defense to prevent the "adding new problems" issue
from happening right now for the 4.16-rc1 merge window.

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sun, Jan 7, 2018 at 1:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
[..]
> Sorry for the confusion, no, I don't mean the "taint tracking", I mean
> the generic pattern of "speculative out of bounds access" that we are
> fixing here.
>
> Yes, as you mentioned before, there are tons of false-positives in the
> tree, as to find the real problems you have to show that userspace
> controls the access index.  But if we have a generic pattern that can
> rewrite that type of logic into one where it does not matter at all
> (i.e. like the ebpf proposed changes), then it would not be an issue if
> they are false or not, we just rewrite them all to be safe.
>
> We need to find some way not only to fix these issues now (like you are
> doing with this series), but to prevent them from every coming back into
> the codebase again.  It's that second part that we need to keep in the
> back of our minds here, while doing the first portion of this work.

I understand the goal, but I'm not sure any of our current annotation
mechanisms are suitable. We have:

    __attribute__((noderef, address_space(x)))

...for the '__user' annotation and other pointers that must not be
de-referenced without a specific accessor. We also have:

    __attribute__((bitwise))

...for values that should not be consumed directly without a specific
conversion like endian swapping.

The problem is that we need to see if a value derived from a userspace
controlled input is used to trigger a chain of dependent reads. As far
as I can see the annotation would need to be guided by taint analysis
to be useful, at which point we can just "annotate" the problem spot
with nospec_array_ptr(). Otherwise it seems the scope of a
"__nospec_array_index" annotation would have a low signal to noise
ratio.

Stopping speculation past a uacess_begin() boundary appears to handle
a wide swath of potential problems, and the rest likely needs taint
analysis, at least for now.

All that to say, yes, we need better tooling and infrastructure going forward.

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 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Saturday, 6 January 2018 03:10:32 EET Dan Williams wrote:
> Static analysis reports that 'index' may be a user controlled value that
> is used as a data dependency to read 'pin' from the
> 'selector->baSourceID' 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 'pin'.

I won't repeat the arguments already made in the thread regarding having 
documented coverity rules for this, even if I agree with them.

> Based on an original patch by Elena Reshetova.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index 3e7e283a44a8..7442626dc20e 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -22,6 +22,7 @@
>  #include <linux/mm.h>
>  #include <linux/wait.h>
>  #include <linux/atomic.h>
> +#include <linux/compiler.h>
> 
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void
> *fh, struct uvc_entity *iterm = NULL;
>  	u32 index = input->index;
>  	int pin = 0;
> +	__u8 *elem;
> 
>  	if (selector == NULL ||
>  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void
> *fh, break;
>  		}
>  		pin = iterm->id;
> -	} else if (index < selector->bNrInPins) {
> -		pin = selector->baSourceID[index];
> +	} else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> +					selector->bNrInPins))) {
> +		pin = *elem;
>  		list_for_each_entry(iterm, &chain->entities, chain) {
>  			if (!UVC_ENTITY_IS_ITERM(iterm))
>  				continue;

(adding a bit more context)

> 			if (iterm->id == pin)
> 				break;
> 		}
> 	}
> 
> 	if (iterm == NULL || iterm->id != pin)
> 		return -EINVAL;
> 
> 	memset(input, 0, sizeof(*input));
> 	input->index = index;
> 	strlcpy(input->name, iterm->name, sizeof(input->name));
> 	if (UVC_ENTITY_TYPE(iterm) == UVC_ITT_CAMERA)
> 		input->type = V4L2_INPUT_TYPE_CAMERA;

So pin is used to search for an entry in the chain->entities list. Entries in 
that list are allocated separately through kmalloc and can thus end up in 
different cache lines, so I agree we have an issue. However, this is mitigated 
by the fact that typical UVC devices have a handful (sometimes up to a dozen) 
entities, so an attacker would only be able to read memory values that are 
equal to the entity IDs used by the device. Entity IDs can be freely allocated 
but typically count continuously from 0. It would take a specially-crafted UVC 
device to be able to read all memory.

On the other hand, as this is nowhere close to being a fast path, I think we 
can close this potential hole as proposed in the patch. So,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Will you merge the whole series in one go, or would you like me to take the 
patch in my tree ? In the latter case I'll wait until the nospec_array_ptr() 
gets merged in mainline.

-- 
Regards,

Laurent Pinchart

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 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Mon, Jan 8, 2018 at 3:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Saturday, 6 January 2018 03:10:32 EET Dan Williams wrote:
>> Static analysis reports that 'index' may be a user controlled value that
>> is used as a data dependency to read 'pin' from the
>> 'selector->baSourceID' 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 'pin'.
>
> I won't repeat the arguments already made in the thread regarding having
> documented coverity rules for this, even if I agree with them.
>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: linux-media@vger.kernel.org
>> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index 3e7e283a44a8..7442626dc20e 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/wait.h>
>>  #include <linux/atomic.h>
>> +#include <linux/compiler.h>
>>
>>  #include <media/v4l2-common.h>
>>  #include <media/v4l2-ctrls.h>
>> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void
>> *fh, struct uvc_entity *iterm = NULL;
>>       u32 index = input->index;
>>       int pin = 0;
>> +     __u8 *elem;
>>
>>       if (selector == NULL ||
>>           (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
>> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void
>> *fh, break;
>>               }
>>               pin = iterm->id;
>> -     } else if (index < selector->bNrInPins) {
>> -             pin = selector->baSourceID[index];
>> +     } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
>> +                                     selector->bNrInPins))) {
>> +             pin = *elem;
>>               list_for_each_entry(iterm, &chain->entities, chain) {
>>                       if (!UVC_ENTITY_IS_ITERM(iterm))
>>                               continue;
>
> (adding a bit more context)
>
>>                       if (iterm->id == pin)
>>                               break;
>>               }
>>       }
>>
>>       if (iterm == NULL || iterm->id != pin)
>>               return -EINVAL;
>>
>>       memset(input, 0, sizeof(*input));
>>       input->index = index;
>>       strlcpy(input->name, iterm->name, sizeof(input->name));
>>       if (UVC_ENTITY_TYPE(iterm) == UVC_ITT_CAMERA)
>>               input->type = V4L2_INPUT_TYPE_CAMERA;
>
> So pin is used to search for an entry in the chain->entities list. Entries in
> that list are allocated separately through kmalloc and can thus end up in
> different cache lines, so I agree we have an issue. However, this is mitigated
> by the fact that typical UVC devices have a handful (sometimes up to a dozen)
> entities, so an attacker would only be able to read memory values that are
> equal to the entity IDs used by the device. Entity IDs can be freely allocated
> but typically count continuously from 0. It would take a specially-crafted UVC
> device to be able to read all memory.
>
> On the other hand, as this is nowhere close to being a fast path, I think we
> can close this potential hole as proposed in the patch. So,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks Laurent!

> Will you merge the whole series in one go, or would you like me to take the
> patch in my tree ? In the latter case I'll wait until the nospec_array_ptr()
> gets merged in mainline.

I'll track it for now. Until the 'nospec_array_ptr()' discussion
resolves there won't be a stabilized commit-id for you to base a
branch.

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Laurent Pinchart]

Hi Greg,

On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> > On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> >> Static analysis reports that 'index' may be a user controlled value that
> >> is used as a data dependency to read 'pin' from the
> >> 'selector->baSourceID' 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 'pin'.
> >> 
> >> Based on an original patch by Elena Reshetova.
> >> 
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> Cc: linux-media@vger.kernel.org
> >> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >> 
> >>  drivers/media/usb/uvc/uvc_v4l2.c |    7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> >> b/drivers/media/usb/uvc/uvc_v4l2.c index 3e7e283a44a8..7442626dc20e
> >> 100644
> >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -22,6 +22,7 @@
> >>  #include <linux/mm.h>
> >>  #include <linux/wait.h>
> >>  #include <linux/atomic.h>
> >> +#include <linux/compiler.h>
> >> 
> >>  #include <media/v4l2-common.h>
> >>  #include <media/v4l2-ctrls.h>
> >> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file,
> >> void *fh,
> >>  	struct uvc_entity *iterm = NULL;
> >>  	u32 index = input->index;
> >>  	int pin = 0;
> >> +	__u8 *elem;
> >> 
> >>  	if (selector == NULL ||
> >>  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> >> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file,
> >> void *fh,
> >>  				break;
> >>  		}
> >>  		pin = iterm->id;
> >> -	} else if (index < selector->bNrInPins) {
> >> -		pin = selector->baSourceID[index];
> >> +	} else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> >> +					selector->bNrInPins))) {
> >> +		pin = *elem;
> > 
> > I dug through this before, and I couldn't find where index came from
> > userspace, I think seeing the coverity rule would be nice.
> 
> Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
> crazy complex (rightfully so), it's amazing that coverity could navigate
> that whole thing :)
> 
> While I'm all for fixing this type of thing, I feel like we need to do
> something "else" for this as playing whack-a-mole for this pattern is
> going to be a never-ending battle for all drivers for forever.

That's my concern too, as even if we managed to find and fix all the 
occurrences of the problematic patterns (and we won't), new ones will keep 
being merged all the time.

> Either we need some way to mark this data path to make it easy for tools
> like sparse to flag easily, or we need to catch the issue in the driver
> subsystems, which unfortunatly, would harm the drivers that don't have
> this type of issue (like here.)

But how would you do so ?

> I'm guessing that other operating systems, which don't have the luxury
> of auditing all of their drivers are going for the "big hammer in the
> subsystem" type of fix, right?

Other operating systems that ship closed-source drivers authored by hardware 
vendors and not reviewed by third parties will likely stay vulnerable forever. 
That's a small concern though as I expect those drivers to contain much large 
security holes anyway.

> I don't have a good answer for this, but if there was some better way to
> rewrite these types of patterns to just prevent the need for the
> nospec_array_ptr() type thing, that might be the best overall for
> everyone.  Much like ebpf did with their changes.  That way a simple
> coccinelle rule would be able to catch the pattern and rewrite it.
> 
> Or am I just dreaming?

Likely, but sometimes dreams come true :-) Do you have an idea how this could 
be done ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Tue, Jan 09, 2018 at 10:40:21AM +0200, Laurent Pinchart wrote:
> On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> > On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> > 
> > While I'm all for fixing this type of thing, I feel like we need to do
> > something "else" for this as playing whack-a-mole for this pattern is
> > going to be a never-ending battle for all drivers for forever.
> 
> That's my concern too, as even if we managed to find and fix all the 
> occurrences of the problematic patterns (and we won't), new ones will keep 
> being merged all the time.

And what about the millions of lines of out-of-tree drivers that we all
rely on every day in our devices?  What about the distro kernels that
add random new drivers?

We need some sort of automated way to scan for this.

Intel, any chance we can get your coverity rules?  Given that the date
of this original patchset was from last August, has anyone looked at
what is now in Linus's tree?  What about linux-next?  I just added 3
brand-new driver subsystems to the kernel tree there, how do we know
there isn't problems in them?

And what about all of the other ways user-data can be affected?  Again,
as Peter pointed out, USB devices.  I want some chance to be able to at
least audit the codebase we have to see if that path is an issue.
Without any hint of how to do this in an automated manner, we are all
in deep shit for forever.

> > Either we need some way to mark this data path to make it easy for tools
> > like sparse to flag easily, or we need to catch the issue in the driver
> > subsystems, which unfortunatly, would harm the drivers that don't have
> > this type of issue (like here.)
> 
> But how would you do so ?

I do not know, it all depends on the access pattern, right?

> > I'm guessing that other operating systems, which don't have the luxury
> > of auditing all of their drivers are going for the "big hammer in the
> > subsystem" type of fix, right?
> 
> Other operating systems that ship closed-source drivers authored by hardware 
> vendors and not reviewed by third parties will likely stay vulnerable forever. 
> That's a small concern though as I expect those drivers to contain much large 
> security holes anyway.

Well yes, but odds are those operating systems are doing something to
mitigate this, right?  Slowing down all user/kernel data paths?
Targeted code analysis tools?  Something else?  I doubt they just don't
care at all about it.  At the least, I would think Coverity would be
trying to sell licenses for this :(

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Laurent Pinchart]

Hi Greg,

On Tuesday, 9 January 2018 12:04:10 EET Greg KH wrote:
> On Tue, Jan 09, 2018 at 10:40:21AM +0200, Laurent Pinchart wrote:
> > On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> >> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> >> 
> >> While I'm all for fixing this type of thing, I feel like we need to do
> >> something "else" for this as playing whack-a-mole for this pattern is
> >> going to be a never-ending battle for all drivers for forever.
> > 
> > That's my concern too, as even if we managed to find and fix all the
> > occurrences of the problematic patterns (and we won't), new ones will keep
> > being merged all the time.
> 
> And what about the millions of lines of out-of-tree drivers that we all
> rely on every day in our devices?  What about the distro kernels that
> add random new drivers?

Of course, even though the out-of-tree drivers probably come with lots of 
security issues worse than this one.

> We need some sort of automated way to scan for this.

Is there any initiative to implement such a scan in an open-source tool ?

We also need to educate developers. An automatic scanner could help there, but 
in the end the information has to spread to all our brains. It won't be easy, 
and is likely not fully feasible, but it's no different than how developers 
have to be educated about race conditions and locking for instance. It's a 
mind set.

> Intel, any chance we can get your coverity rules?  Given that the date
> of this original patchset was from last August, has anyone looked at
> what is now in Linus's tree?  What about linux-next?  I just added 3
> brand-new driver subsystems to the kernel tree there, how do we know
> there isn't problems in them?
> 
> And what about all of the other ways user-data can be affected?  Again,
> as Peter pointed out, USB devices.  I want some chance to be able to at
> least audit the codebase we have to see if that path is an issue.
> Without any hint of how to do this in an automated manner, we are all
> in deep shit for forever.

Or at least until the hardware architecture evolves. Let's drop the x86 
instruction set, expose the µops, and have gcc handle the scheduling. Sure, it 
will mean recompiling everything for every x86 CPU model out there, but we 
have source-based distros to the rescue :-D

> >> Either we need some way to mark this data path to make it easy for tools
> >> like sparse to flag easily, or we need to catch the issue in the driver
> >> subsystems, which unfortunatly, would harm the drivers that don't have
> >> this type of issue (like here.)
> > 
> > But how would you do so ?
> 
> I do not know, it all depends on the access pattern, right?

Any data coming from userspace could trigger such accesses. If we want 
complete coverage the only way I can think of is starting from syscalls and 
tainting data down the call stacks (__user could help to some extend), but 
we'll likely be drowned in false positives. I don't see how we could mark 
paths manually.

> >> I'm guessing that other operating systems, which don't have the luxury
> >> of auditing all of their drivers are going for the "big hammer in the
> >> subsystem" type of fix, right?
> > 
> > Other operating systems that ship closed-source drivers authored by
> > hardware vendors and not reviewed by third parties will likely stay
> > vulnerable forever. That's a small concern though as I expect those
> > drivers to contain much large security holes anyway.
> 
> Well yes, but odds are those operating systems are doing something to
> mitigate this, right?  Slowing down all user/kernel data paths?
> Targeted code analysis tools?  Something else?  I doubt they just don't
> care at all about it.  At the least, I would think Coverity would be
> trying to sell licenses for this :(

Given their track record of security issues in drivers (and I won't even 
mention the ones that are present by design, such as root kits in game copy 
protection systems for instance) I doubt they will do much beside sprinkling a 
bit of PR dust, at least for the consumer market. On the server market it 
might be different as there's less hardware variation, and thus less drivers 
to handle.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Tue, Jan 09, 2018 at 04:26:28PM +0200, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Tuesday, 9 January 2018 12:04:10 EET Greg KH wrote:
> > On Tue, Jan 09, 2018 at 10:40:21AM +0200, Laurent Pinchart wrote:
> > > On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> > >> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> > >> 
> > >> While I'm all for fixing this type of thing, I feel like we need to do
> > >> something "else" for this as playing whack-a-mole for this pattern is
> > >> going to be a never-ending battle for all drivers for forever.
> > > 
> > > That's my concern too, as even if we managed to find and fix all the
> > > occurrences of the problematic patterns (and we won't), new ones will keep
> > > being merged all the time.
> > 
> > And what about the millions of lines of out-of-tree drivers that we all
> > rely on every day in our devices?  What about the distro kernels that
> > add random new drivers?
> 
> Of course, even though the out-of-tree drivers probably come with lots of 
> security issues worse than this one.

Sure, but I have worked with some teams that have used coverity to find
and fix all of the reported bugs it founds.  So some companies are
trying to fix their problems here, let's not make it impossible for them :)

> > We need some sort of automated way to scan for this.
> 
> Is there any initiative to implement such a scan in an open-source tool ?

Sure, if you want to, but I have no such initiative...

> We also need to educate developers. An automatic scanner could help there, but 
> in the end the information has to spread to all our brains. It won't be easy, 
> and is likely not fully feasible, but it's no different than how developers 
> have to be educated about race conditions and locking for instance. It's a 
> mind set.

Agreed.

> > Intel, any chance we can get your coverity rules?  Given that the date
> > of this original patchset was from last August, has anyone looked at
> > what is now in Linus's tree?  What about linux-next?  I just added 3
> > brand-new driver subsystems to the kernel tree there, how do we know
> > there isn't problems in them?
> > 
> > And what about all of the other ways user-data can be affected?  Again,
> > as Peter pointed out, USB devices.  I want some chance to be able to at
> > least audit the codebase we have to see if that path is an issue.
> > Without any hint of how to do this in an automated manner, we are all
> > in deep shit for forever.
> 
> Or at least until the hardware architecture evolves. Let's drop the x86 
> instruction set, expose the µops, and have gcc handle the scheduling. Sure, it 
> will mean recompiling everything for every x86 CPU model out there, but we 
> have source-based distros to the rescue :-D

Then we are back at the itanium mess, where all of the hardware issues
were supposed be fixed by the compiler writers.  We all remember how
well that worked out...

> > >> Either we need some way to mark this data path to make it easy for tools
> > >> like sparse to flag easily, or we need to catch the issue in the driver
> > >> subsystems, which unfortunatly, would harm the drivers that don't have
> > >> this type of issue (like here.)
> > > 
> > > But how would you do so ?
> > 
> > I do not know, it all depends on the access pattern, right?
> 
> Any data coming from userspace could trigger such accesses. If we want 
> complete coverage the only way I can think of is starting from syscalls and 
> tainting data down the call stacks (__user could help to some extend), but 
> we'll likely be drowned in false positives. I don't see how we could mark 
> paths manually.

I agree, which is why I want to see how someone did this work
originally.  We have no idea as no one is telling us anything :(

How do we "know" that these are the only problem areas?  When was the
last scan run?  On what tree?  And so on...

thanks,

greg k-h

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.