| Message ID | 151520103240.32271.14706852449205864676.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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
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.
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.
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.
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 ?
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
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.
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
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;