| Message ID | 3d4973141e218fb516422d3d831742d55aaa5c04.1524499368.git.gustavo@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Mon, 23 Apr 2018 12:38:03 -0500 "Gustavo A. R. Silva" <gustavo@embeddedor.com> 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@vger.kernel.org > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > 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
On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote: > Em Mon, 23 Apr 2018 12:38:03 -0500 > "Gustavo A. R. Silva" <gustavo@embeddedor.com> 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@vger.kernel.org >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> 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. > I see I've missed some obvious things that you've pointed out here. I'll mark these warnings as False Positives and take your points into account for the analysis of the rest of the Spectre issues reported by Smatch. Sorry for the noise and thanks for the feedback. Thanks -- Gustavo
Em Mon, 23 Apr 2018 14:11:02 -0500 "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu: > On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 23 Apr 2018 12:38:03 -0500 > > "Gustavo A. R. Silva" <gustavo@embeddedor.com> 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@vger.kernel.org > >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > >> --- > >> 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. > > > I see I've missed some obvious things that you've pointed out here. I'll > mark these warnings as False Positives and take your points into account > for the analysis of the rest of the Spectre issues reported by Smatch. Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org. Please feel free to resubmit any patch if they represent a real threat, adding a corresponding description about the threat scenario at the body of the e-mail. > Sorry for the noise and thanks for the feedback. Anytime. Thanks, Mauro
On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote: > Em Mon, 23 Apr 2018 14:11:02 -0500 > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu: > >> On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote: >>> Em Mon, 23 Apr 2018 12:38:03 -0500 >>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> 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@vger.kernel.org >>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>>> --- >>>> 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. >>> >> I see I've missed some obvious things that you've pointed out here. I'll >> mark these warnings as False Positives and take your points into account >> for the analysis of the rest of the Spectre issues reported by Smatch. > > Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org. > Please feel free to resubmit any patch if they represent a real > threat, adding a corresponding description about the threat scenario > at the body of the e-mail. > Yeah. I got it. >> Sorry for the noise and thanks for the feedback. > > Anytime. > Much appreciated. :) Thanks -- Gustavo
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? 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@embeddedor.com> 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@vger.kernel.org > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > --- > > 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
Hi Dan, Em Tue, 24 Apr 2018 12:35:00 +0300 Dan Carpenter <dan.carpenter@oracle.com> 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@embeddedor.com> 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@vger.kernel.org > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > > --- > > > 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
On Tue, Apr 24, 2018 at 12:35:00PM +0300, Dan Carpenter wrote: > 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@embeddedor.com> escreveu: > > > @@ -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? TL;DR: read the papers [1] & [2] I suspect you didn't get the gist of Spectre V1 [1], let me explain: Suppose userspace provides f->index > ARRAY_SIZE(format), and we predict the branch to -EINVAL to not be taken. Then the CPU _WILL_ load (out of bounds) format[f->index] into f->pixelformat and continue onwards to use this bogus value, all the way until it figures out the branch was mis-predicted. Once it figures out the mispredict, it will throw away the state and start over at the condition site. So far, so basic. The thing is, is will not (and cannot) throw away all state. Suppose our speculation continues into v4l_fill_fmtdesc() and that switch there is compiled as another array lookup, it will then feed our f->pixelformat (which contains random kernel memory) into that array to find the requested descr pointer. Now, imagine userspace having flushed cache on the descr pointer array, having trained the branch predictor to mis-predict the branch (see branchscope paper [2]) and doing that out-of-bounds ioctl(). It can then speculative do the out-of-bounds array access, followed by the desc array load, then figure out it was wrong and redo. Then usespace probes which part of the descr[] array is now in cache and from that it can infer the initial out-of-bound value. So while format[] is static and bound, it can read random kernel memory up to format+4g, including your crypto keys. As far as V1 goes, this is actually a fairly solid exploit candidate. No false positive about it. Now kernel policy is to kill any and all speculation on user controlled array indexing such that we don't have to go look for subsequent side channels (the above cache side channel is the one described in the Spectre paper and by far the easiest, but there are other possible side channels) and we simply don't want to worry about it. So even from that pov, the proposed patch is good. [1] https://spectreattack.com/spectre.pdf [2] www.cs.ucr.edu/~nael/pubs/asplos18.pdf
On Tue, Apr 24, 2018 at 12:36:09PM +0200, Peter Zijlstra wrote: > > Then usespace probes which part of the descr[] array is now in cache and > from that it can infer the initial out-of-bound value. Just had a better look at v4l_fill_fmtdesc() and actually read the comment. The code cannot be compiled as a array because it is big and sparse. But the log(n) condition tree is a prime candidate for the branchscope side-channel, which would be able to reconstruct a significant number of bits of the original value. A denser tree gives more bits etc.
Em Tue, 24 Apr 2018 12:36:09 +0200 Peter Zijlstra <peterz@infradead.org> escreveu: > On Tue, Apr 24, 2018 at 12:35:00PM +0300, Dan Carpenter wrote: > > 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@embeddedor.com> escreveu: > > > > > @@ -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? > > TL;DR: read the papers [1] & [2] > > I suspect you didn't get the gist of Spectre V1 [1], let me explain: > > Suppose userspace provides f->index > ARRAY_SIZE(format), and we predict > the branch to -EINVAL to not be taken. > > Then the CPU _WILL_ load (out of bounds) format[f->index] into > f->pixelformat and continue onwards to use this bogus value, all the way > until it figures out the branch was mis-predicted. > > Once it figures out the mispredict, it will throw away the state and > start over at the condition site. So far, so basic. > > The thing is, is will not (and cannot) throw away all state. Suppose our > speculation continues into v4l_fill_fmtdesc() and that switch there is > compiled as another array lookup, it will then feed our f->pixelformat > (which contains random kernel memory) into that array to find the > requested descr pointer. > > Now, imagine userspace having flushed cache on the descr pointer array, > having trained the branch predictor to mis-predict the branch (see > branchscope paper [2]) and doing that out-of-bounds ioctl(). > > It can then speculative do the out-of-bounds array access, followed by > the desc array load, then figure out it was wrong and redo. > > Then usespace probes which part of the descr[] array is now in cache and > from that it can infer the initial out-of-bound value. > > So while format[] is static and bound, it can read random kernel memory > up to format+4g, including your crypto keys. > > As far as V1 goes, this is actually a fairly solid exploit candidate. No > false positive about it. > > Now kernel policy is to kill any and all speculation on user controlled > array indexing such that we don't have to go look for subsequent side > channels (the above cache side channel is the one described in the > Spectre paper and by far the easiest, but there are other possible side > channels) and we simply don't want to worry about it. > > So even from that pov, the proposed patch is good. > > > [1] https://spectreattack.com/spectre.pdf > [2] www.cs.ucr.edu/~nael/pubs/asplos18.pdf > On Tue, Apr 24, 2018 at 12:36:09PM +0200, Peter Zijlstra wrote: > > > > Then usespace probes which part of the descr[] array is now in cache and > > from that it can infer the initial out-of-bound value. > > Just had a better look at v4l_fill_fmtdesc() and actually read the > comment. The code cannot be compiled as a array because it is big and > sparse. But the log(n) condition tree is a prime candidate for the > branchscope side-channel, which would be able to reconstruct a > significant number of bits of the original value. A denser tree gives > more bits etc. Peter, Thanks for a comprehensive explanation about that. It now makes more sense to me. Yeah, better to apply a fix to avoid the issue with VIDIOC_ENUM_FMT. Btw, on almost all media drivers, the implementation for enumerating the supported formats are the same (and we have a few other VIDOC_ENUM_foo ioctls that usually do similar stuff): the V4L2 core calls a driver, with looks into an array, returning the results to the core. So, a fix like that should likely go to almost all media drivers (there are a lot of them!), and, for every new one, to take care to avoid introducing it again during patch review process. So, I'm wondering if are there any way to mitigate it inside the core itself, instead of doing it on every driver, e. g. changing v4l_enum_fmt() implementation at v4l2-ioctl. Ok, a "poor man" approach would be to pass the array directly to the core and let the implementation there to implement the array fetch logic, calling array_index_nospec() there, but I wonder if are there any other way that won't require too much code churn. Thanks, Mauro
On Tue, Apr 24, 2018 at 02:47:55PM -0300, Mauro Carvalho Chehab wrote: > So, I'm wondering if are there any way to mitigate it inside the > core itself, instead of doing it on every driver, e. g. changing > v4l_enum_fmt() implementation at v4l2-ioctl. > > Ok, a "poor man" approach would be to pass the array directly to > the core and let the implementation there to implement the array > fetch logic, calling array_index_nospec() there, but I wonder if > are there any other way that won't require too much code churn. Sadly no; the whole crux is the array bound check itself. You could maybe pass around the array size to the core code and then do something like: if (f->index >= f->array_size) return -EINVAL; f->index = nospec_array_index(f->index, f->array_size); in generic code, and have all the drivers use f->index as usual, but even that would be quite a bit of code churn I guess.
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)); strlcpy(f->description, format[f->index].name, sizeof(f->description)); f->pixelformat = format[f->index].fourcc; return 0;
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@vger.kernel.org Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/media/usb/tm6000/tm6000-video.c | 2 ++ 1 file changed, 2 insertions(+)