Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Hi Dan,

Em Tue, 24 Apr 2018 12:35:00 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> escreveu:

> Hi Mauro,
> 
> I saw your comment on LWN.  You argue on LWN that since the format array
> is static the CPU won't speculatively read past the L1 cache?

The intent of that comment is to be provocative, in the sense that
people would argue against and point flaws (if any) on my rationale.

As I explained when reviewing this patch, I don't care much if an
automatic tool is saying that there's a vulnerability at the code,
as it could be a false positive. So, what I want at the patch description
is a threat analysis explaining how an algorithm is exploited.

With regards to Spectre, I never tried to write an exploit myself, nor
had to study it in detail in order to mitigate it. So, what I know about
it is what I read on a few places. From the places where I read, the 
boundaries for an array exploit are limited to L1 cache, but,
as I said before, I can be wrong on that.

It will be great to hear to Peter's comment on that, as he knows a
lot more than me about it.
> 
> I don't know if that's true.  It should be easy enough to filter out
> the reads into static arrays.  Peter do you know the answer here?

> 
> regards,
> dan carpenter
> 
> On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 23 Apr 2018 12:38:03 -0500
> > "Gustavo A. R. Silva" <gustavo@xxxxxxxxxxxxxx> escreveu:
> >   
> > > f->index can be controlled by user-space, hence leading to a
> > > potential exploitation of the Spectre variant 1 vulnerability.
> > > 
> > > Smatch warning:
> > > drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'
> > > 
> > > Fix this by sanitizing f->index before using it to index
> > > array _format_
> > > 
> > > Notice that given that speculation windows are large, the policy is
> > > to kill the speculation on the first load and not worry if it can be
> > > completed with a dependent load/store [1].
> > > 
> > > [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Signed-off-by: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
> > > index b2399d4..d701027 100644
> > > --- a/drivers/media/usb/tm6000/tm6000-video.c
> > > +++ b/drivers/media/usb/tm6000/tm6000-video.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/kthread.h>
> > >  #include <linux/highmem.h>
> > >  #include <linux/freezer.h>
> > > +#include <linux/nospec.h>
> > >  
> > >  #include "tm6000-regs.h"
> > >  #include "tm6000.h"
> > > @@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
> > >  	if (f->index >= ARRAY_SIZE(format))
> > >  		return -EINVAL;
> > >  
> > > +	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));  
> >
> > Please enlighten me: how do you think this could be exploited?
> > 
> > When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device,
> > it will just enumerate a hardware functionality, with is constant
> > for a given hardware piece.
> > 
> > The way it works is that userspace do something like:
> > 
> > 	int ret = 0;
> > 
> > 	for (i = 0; ret == 0; i++) {
> > 		ret = ioctl(VIDIOC_ENUM_FMT, ...);
> > 	}
> > 
> > in order to read an entire const table.
> > 
> > Usually, it doesn't require any special privilege to call this ioctl,
> > but, even if someone changes its permission to 0x400, a simple lsusb
> > output is enough to know what hardware model is there. A lsmod
> > or cat /proc/modules) also tells that the tm6000 module was loaded,
> > with is a very good hint that the tm6000 is there or was there in the
> > past.
> > 
> > In the specific case of tm6000, all hardware supports exactly the
> > same formats, as this is usually defined per-driver. So, a quick look
> > at the driver is enough to know exactly what the ioctl would answer. 
> > Also, the net is full of other resources that would allow anyone
> > to get the supported formats for a piece of hardware.
> > 
> > Even assuming that the OS doesn't have lsusb, that /proc is not
> > mounted, that /dev/video0 require special permissions, that the
> > potential attacker doesn't have physical access to the equipment (in
> > order to see if an USB board is plugged), etc... What possible harm
> > he could do by identifying a hardware feature?
> > 
> > Similar notes for the other patches to drivers/media in this
> > series: let's not just start adding bloatware where not needed.
> > 
> > Please notice that I'm fine if you want to submit potential
> > Spectre variant 1 fixups, but if you're willing to do so,
> > please provide an explanation about the potential threat scenarios
> > that you're identifying at the code.
> > 
> > Dan,
> > 
> > It probably makes sense to have somewhere at smatch a place where
> > we could explicitly mark the false-positives, in order to avoid
> > use to receive patches that would just add an extra delay where
> > it is not needed.
> > 
> > Regards,
> > Mauro  



Thanks,
Mauro