| Message ID | 1345864146-2207-9-git-send-email-elezegarcia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 25 Aug 2012 00:09:06 -0300 Ezequiel Garcia <elezegarcia@gmail.com> wrote: > -int vb2_queue_init(struct vb2_queue *q) > +void vb2_queue_init(struct vb2_queue *q) > { > BUG_ON(!q); > BUG_ON(!q->ops); If this change goes through in this form, you can add my ack for the Marvell piece. But I have to wonder...might it not be better to retain the return value and use it to return -EINVAL instead of the seven BUG_ON() calls found in that function? It shouldn't be necessary to bring things down in this situation, and, who knows, one of those might just be turned into a DOS vector with some driver someday. jon -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(Ccing videobuf2 authors) On Sat, Aug 25, 2012 at 12:28 PM, Jonathan Corbet <corbet@lwn.net> wrote: > On Sat, 25 Aug 2012 00:09:06 -0300 > Ezequiel Garcia <elezegarcia@gmail.com> wrote: > >> -int vb2_queue_init(struct vb2_queue *q) >> +void vb2_queue_init(struct vb2_queue *q) >> { >> BUG_ON(!q); >> BUG_ON(!q->ops); > > If this change goes through in this form, you can add my ack for the > Marvell piece. But I have to wonder...might it not be better to retain > the return value and use it to return -EINVAL instead of the seven BUG_ON() > calls found in that function? It shouldn't be necessary to bring things > down in this situation, and, who knows, one of those might just be turned > into a DOS vector with some driver someday. > The mentioned BUG_ON() are these: void vb2_queue_init(struct vb2_queue *q) { BUG_ON(!q); BUG_ON(!q->ops); BUG_ON(!q->mem_ops); BUG_ON(!q->type); BUG_ON(!q->io_modes); [...] Unless I'm overlooking something they look fine to me, since vb2_queue should always be prepared by the driver. On the other hand, it seems these BUG_ON are inherited from videobuf1 (see videobuf_queue_core_init). Marek, Pawel: What do you think? Thanks, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 25 Aug 2012 13:12:01 -0300 Ezequiel Garcia <elezegarcia@gmail.com> wrote: > The mentioned BUG_ON() are these: > > void vb2_queue_init(struct vb2_queue *q) > { > BUG_ON(!q); > BUG_ON(!q->ops); > BUG_ON(!q->mem_ops); > BUG_ON(!q->type); > BUG_ON(!q->io_modes); > [...] > > Unless I'm overlooking something they look fine to me, > since vb2_queue should always be prepared by the driver. http://permalink.gmane.org/gmane.linux.kernel/1347333 is, I believe, the definitive word on this kind of use of BUG_ON()... jon -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, On Sat, Aug 25, 2012 at 2:30 PM, Jonathan Corbet <corbet@lwn.net> wrote: > On Sat, 25 Aug 2012 13:12:01 -0300 > Ezequiel Garcia <elezegarcia@gmail.com> wrote: > >> The mentioned BUG_ON() are these: >> >> void vb2_queue_init(struct vb2_queue *q) >> { >> BUG_ON(!q); >> BUG_ON(!q->ops); >> BUG_ON(!q->mem_ops); >> BUG_ON(!q->type); >> BUG_ON(!q->io_modes); >> [...] >> >> Unless I'm overlooking something they look fine to me, >> since vb2_queue should always be prepared by the driver. > > http://permalink.gmane.org/gmane.linux.kernel/1347333 is, I believe, the > definitive word on this kind of use of BUG_ON()... > As usual Linus's words are truly enlightening. Perhaps you could help me understand this better: 1. Why do we need to check for all these conditions in the first place? There are many other functions relying on "struct vb2_queue *q" not being null (almost all of them) and we don't check for it. What makes vb2_queue_init() so special that we need to check for it? 2. If a DoS attack is the concern here, I wonder how this would be achieved? vb2_queue_init() is an "internal" (so to speak) function, that will only be called by videobuf2 drivers. I'm not arguing, truly. I wan't to understand what's the rationale behind putting BUG_ON, or WARN_ON, or return -EINVAL in a case like this. Thanks, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 26 Aug 2012 19:59:40 -0300 Ezequiel Garcia <elezegarcia@gmail.com> wrote: > 1. > Why do we need to check for all these conditions in the first place? > There are many other functions relying on "struct vb2_queue *q" > not being null (almost all of them) and we don't check for it. > What makes vb2_queue_init() so special that we need to check for it? There are plenty of developers who would argue for the removal of the BUG_ON(!q) line regardless, since the kernel will quickly crash shortly thereafter. I'm a bit less convinced; there are attackers who are very good at exploiting null pointer dereferences, and some systems still allow the low part of the address space to be mapped. In general, IMO, checks for consistency make sense; it's nice if the kernel can *tell* you that something is wrong. What's a mistake is the BUG_ON; that should really only be used in places where things simply cannot continue. In this case, the initialization can be failed, the V4L2 device will likely be unavailable, but everything else can continue as normal. -EINVAL is the right response here. > 2. > If a DoS attack is the concern here, I wonder how this would be achieved? > vb2_queue_init() is an "internal" (so to speak) function, that will only > be called by videobuf2 drivers. It would depend on a driver bug, but the sad fact is that driver bugs do exist. Perhaps it's as simple as getting the driver module to load when the hardware is absent, for example. > I'm not arguing, truly. I wan't to understand what's the rationale behind > putting BUG_ON, or WARN_ON, or return -EINVAL in a case like this. In short: we want the kernel to be as robust as it can be. Detecting problems before they can snowball helps in that regard. Hitting the big red BUG_ON() button in situations where things can continue does not. At least, that's how I see it. jon -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, Thanks for your answers, I really appreciate it. On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote: > On Sun, 26 Aug 2012 19:59:40 -0300 > Ezequiel Garcia <elezegarcia@gmail.com> wrote: > >> 1. >> Why do we need to check for all these conditions in the first place? >> There are many other functions relying on "struct vb2_queue *q" >> not being null (almost all of them) and we don't check for it. >> What makes vb2_queue_init() so special that we need to check for it? > > There are plenty of developers who would argue for the removal of the > BUG_ON(!q) line regardless, since the kernel will quickly crash shortly > thereafter. I'm a bit less convinced; there are attackers who are very > good at exploiting null pointer dereferences, and some systems still allow > the low part of the address space to be mapped. > > In general, IMO, checks for consistency make sense; it's nice if the > kernel can *tell* you that something is wrong. > > What's a mistake is the BUG_ON; that should really only be used in places > where things simply cannot continue. In this case, the initialization can > be failed, the V4L2 device will likely be unavailable, but everything else > can continue as normal. -EINVAL is the right response here. > I see your point. What I really can't seem to understand is why we should have a check at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one. Thanks a lot! Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 28-08-2012 14:23, Ezequiel Garcia escreveu: > Hi Jon, > > Thanks for your answers, I really appreciate it. > > On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote: >> On Sun, 26 Aug 2012 19:59:40 -0300 >> Ezequiel Garcia <elezegarcia@gmail.com> wrote: >> >>> 1. >>> Why do we need to check for all these conditions in the first place? >>> There are many other functions relying on "struct vb2_queue *q" >>> not being null (almost all of them) and we don't check for it. >>> What makes vb2_queue_init() so special that we need to check for it? >> >> There are plenty of developers who would argue for the removal of the >> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly >> thereafter. I'm a bit less convinced; there are attackers who are very >> good at exploiting null pointer dereferences, and some systems still allow >> the low part of the address space to be mapped. >> >> In general, IMO, checks for consistency make sense; it's nice if the >> kernel can *tell* you that something is wrong. >> >> What's a mistake is the BUG_ON; that should really only be used in places >> where things simply cannot continue. In this case, the initialization can >> be failed, the V4L2 device will likely be unavailable, but everything else >> can continue as normal. -EINVAL is the right response here. >> > > I see your point. > > What I really can't seem to understand is why we should have a check > at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one. Those BUG_ON() checks are there since likely the first version of VB1. VB2 just inherited it. The think is that letting the VB code to run without checking for some conditions is evil, as it could cause mass memory corruption, as the videobuf code writes on a large amount of memory (typically, something like 512MB written on every 1/30s). So, the code has protections, in order to try avoiding it. Even so, with VB1, when the output buffer is at the video adapter memory region (what is called PCI2PCI memory transfers), there are known bugs with some chipsets that will cause mass data corruption at the hard disks (as the PCI2PCI transfers interfere at the data transfers from/to the disk, due to hardware bugs). Calling WARN_ON_ONCE() and returning some error code works, provided that we enforce that the error code will be handled at the drivers that call vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull)) and double_checking the code at VB2 callers. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Em 28-08-2012 14:23, Ezequiel Garcia escreveu: >> Hi Jon, >> >> Thanks for your answers, I really appreciate it. >> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote: >>> On Sun, 26 Aug 2012 19:59:40 -0300 >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote: >>> >>>> 1. >>>> Why do we need to check for all these conditions in the first place? >>>> There are many other functions relying on "struct vb2_queue *q" >>>> not being null (almost all of them) and we don't check for it. >>>> What makes vb2_queue_init() so special that we need to check for it? >>> >>> There are plenty of developers who would argue for the removal of the >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly >>> thereafter. I'm a bit less convinced; there are attackers who are very >>> good at exploiting null pointer dereferences, and some systems still allow >>> the low part of the address space to be mapped. >>> >>> In general, IMO, checks for consistency make sense; it's nice if the >>> kernel can *tell* you that something is wrong. >>> >>> What's a mistake is the BUG_ON; that should really only be used in places >>> where things simply cannot continue. In this case, the initialization can >>> be failed, the V4L2 device will likely be unavailable, but everything else >>> can continue as normal. -EINVAL is the right response here. >>> >> >> I see your point. >> >> What I really can't seem to understand is why we should have a check >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one. > > Those BUG_ON() checks are there since likely the first version of VB1. > VB2 just inherited it. > > The think is that letting the VB code to run without checking for some > conditions is evil, as it could cause mass memory corruption, as the > videobuf code writes on a large amount of memory (typically, something > like 512MB written on every 1/30s). So, the code has protections, in order > to try avoiding it. Even so, with VB1, when the output buffer is at the > video adapter memory region (what is called PCI2PCI memory transfers), > there are known bugs with some chipsets that will cause mass data corruption > at the hard disks (as the PCI2PCI transfers interfere at the data transfers > from/to the disk, due to hardware bugs). > > Calling WARN_ON_ONCE() and returning some error code works, provided that > we enforce that the error code will be handled at the drivers that call > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull)) > and double_checking the code at VB2 callers. > So, you want me to resend? And this whole patchset patchset should be dropped because we'll return something from vb2_queue_init. Thanks, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mauro, (Cc got messed up, so I'm sending this to you and media list) On Sat, Sep 15, 2012 at 10:37 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Em Sat, 15 Sep 2012 10:05:40 -0300 > Ezequiel Garcia <elezegarcia@gmail.com> escreveu: > >> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab >> <mchehab@redhat.com> wrote: >> > Em 28-08-2012 14:23, Ezequiel Garcia escreveu: >> >> Hi Jon, >> >> >> >> Thanks for your answers, I really appreciate it. >> >> >> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote: >> >>> On Sun, 26 Aug 2012 19:59:40 -0300 >> >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote: >> >>> >> >>>> 1. >> >>>> Why do we need to check for all these conditions in the first place? >> >>>> There are many other functions relying on "struct vb2_queue *q" >> >>>> not being null (almost all of them) and we don't check for it. >> >>>> What makes vb2_queue_init() so special that we need to check for it? >> >>> >> >>> There are plenty of developers who would argue for the removal of the >> >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly >> >>> thereafter. I'm a bit less convinced; there are attackers who are very >> >>> good at exploiting null pointer dereferences, and some systems still allow >> >>> the low part of the address space to be mapped. >> >>> >> >>> In general, IMO, checks for consistency make sense; it's nice if the >> >>> kernel can *tell* you that something is wrong. >> >>> >> >>> What's a mistake is the BUG_ON; that should really only be used in places >> >>> where things simply cannot continue. In this case, the initialization can >> >>> be failed, the V4L2 device will likely be unavailable, but everything else >> >>> can continue as normal. -EINVAL is the right response here. >> >>> >> >> >> >> I see your point. >> >> >> >> What I really can't seem to understand is why we should have a check >> >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one. >> > >> > Those BUG_ON() checks are there since likely the first version of VB1. >> > VB2 just inherited it. >> > >> > The think is that letting the VB code to run without checking for some >> > conditions is evil, as it could cause mass memory corruption, as the >> > videobuf code writes on a large amount of memory (typically, something >> > like 512MB written on every 1/30s). So, the code has protections, in order >> > to try avoiding it. Even so, with VB1, when the output buffer is at the >> > video adapter memory region (what is called PCI2PCI memory transfers), >> > there are known bugs with some chipsets that will cause mass data corruption >> > at the hard disks (as the PCI2PCI transfers interfere at the data transfers >> > from/to the disk, due to hardware bugs). >> > >> > Calling WARN_ON_ONCE() and returning some error code works, provided that >> > we enforce that the error code will be handled at the drivers that call >> > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull)) >> > and double_checking the code at VB2 callers. >> > >> >> So, you want me to resend? > > Yes, please. > I can't decide on coding style. So, how about this?: (copy pasted on gmail, sorry if spaces are mangled): */ int vb2_queue_init(struct vb2_queue *q) { - BUG_ON(!q); - BUG_ON(!q->ops); - BUG_ON(!q->mem_ops); - BUG_ON(!q->type); - BUG_ON(!q->io_modes); - - BUG_ON(!q->ops->queue_setup); - BUG_ON(!q->ops->buf_queue); + /* + * Sanity check + */ + if (WARN_ON_ONCE(!q) || + WARN_ON_ONCE(!q->ops) || + WARN_ON_ONCE(!q->mem_ops) || + WARN_ON_ONCE(!q->type) || + WARN_ON_ONCE(!q->io_modes) || + WARN_ON_ONCE(!q->ops->queue_setup) || + WARN_ON_ONCE(!q->ops->buf_queue)) + return -EINVAL; INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 15 Sep 2012 12:22:48 -0300 Ezequiel Garcia <elezegarcia@gmail.com> escreveu: > Mauro, > > (Cc got messed up, so I'm sending this to you and media list) > > On Sat, Sep 15, 2012 at 10:37 AM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: > > Em Sat, 15 Sep 2012 10:05:40 -0300 > > Ezequiel Garcia <elezegarcia@gmail.com> escreveu: > > > >> On Sat, Sep 15, 2012 at 9:48 AM, Mauro Carvalho Chehab > >> <mchehab@redhat.com> wrote: > >> > Em 28-08-2012 14:23, Ezequiel Garcia escreveu: > >> >> Hi Jon, > >> >> > >> >> Thanks for your answers, I really appreciate it. > >> >> > >> >> On Tue, Aug 28, 2012 at 1:55 PM, Jonathan Corbet <corbet@lwn.net> wrote: > >> >>> On Sun, 26 Aug 2012 19:59:40 -0300 > >> >>> Ezequiel Garcia <elezegarcia@gmail.com> wrote: > >> >>> > >> >>>> 1. > >> >>>> Why do we need to check for all these conditions in the first place? > >> >>>> There are many other functions relying on "struct vb2_queue *q" > >> >>>> not being null (almost all of them) and we don't check for it. > >> >>>> What makes vb2_queue_init() so special that we need to check for it? > >> >>> > >> >>> There are plenty of developers who would argue for the removal of the > >> >>> BUG_ON(!q) line regardless, since the kernel will quickly crash shortly > >> >>> thereafter. I'm a bit less convinced; there are attackers who are very > >> >>> good at exploiting null pointer dereferences, and some systems still allow > >> >>> the low part of the address space to be mapped. > >> >>> > >> >>> In general, IMO, checks for consistency make sense; it's nice if the > >> >>> kernel can *tell* you that something is wrong. > >> >>> > >> >>> What's a mistake is the BUG_ON; that should really only be used in places > >> >>> where things simply cannot continue. In this case, the initialization can > >> >>> be failed, the V4L2 device will likely be unavailable, but everything else > >> >>> can continue as normal. -EINVAL is the right response here. > >> >>> > >> >> > >> >> I see your point. > >> >> > >> >> What I really can't seem to understand is why we should have a check > >> >> at vb2_queue_init() but not at vb2_get_drv_priv(), just to pick one. > >> > > >> > Those BUG_ON() checks are there since likely the first version of VB1. > >> > VB2 just inherited it. > >> > > >> > The think is that letting the VB code to run without checking for some > >> > conditions is evil, as it could cause mass memory corruption, as the > >> > videobuf code writes on a large amount of memory (typically, something > >> > like 512MB written on every 1/30s). So, the code has protections, in order > >> > to try avoiding it. Even so, with VB1, when the output buffer is at the > >> > video adapter memory region (what is called PCI2PCI memory transfers), > >> > there are known bugs with some chipsets that will cause mass data corruption > >> > at the hard disks (as the PCI2PCI transfers interfere at the data transfers > >> > from/to the disk, due to hardware bugs). > >> > > >> > Calling WARN_ON_ONCE() and returning some error code works, provided that > >> > we enforce that the error code will be handled at the drivers that call > >> > vb2_queue_init(), using something like __attribute__((warn_unused_result, nonnull)) > >> > and double_checking the code at VB2 callers. > >> > > >> > >> So, you want me to resend? > > > > Yes, please. > > > > I can't decide on coding style. So, how about this?: > > (copy pasted on gmail, sorry if spaces are mangled): > > */ > int vb2_queue_init(struct vb2_queue *q) > { > - BUG_ON(!q); > - BUG_ON(!q->ops); > - BUG_ON(!q->mem_ops); > - BUG_ON(!q->type); > - BUG_ON(!q->io_modes); > - > - BUG_ON(!q->ops->queue_setup); > - BUG_ON(!q->ops->buf_queue); > + /* > + * Sanity check > + */ > + if (WARN_ON_ONCE(!q) || > + WARN_ON_ONCE(!q->ops) || > + WARN_ON_ONCE(!q->mem_ops) || > + WARN_ON_ONCE(!q->type) || > + WARN_ON_ONCE(!q->io_modes) || > + WARN_ON_ONCE(!q->ops->queue_setup) || > + WARN_ON_ONCE(!q->ops->buf_queue)) > + return -EINVAL; > > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); It seems OK on my eyes.
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 4da3df6..ea45842 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1736,7 +1736,7 @@ EXPORT_SYMBOL_GPL(vb2_poll); * to the struct vb2_queue description in include/media/videobuf2-core.h * for more information. */ -int vb2_queue_init(struct vb2_queue *q) +void vb2_queue_init(struct vb2_queue *q) { BUG_ON(!q); BUG_ON(!q->ops); @@ -1755,7 +1755,6 @@ int vb2_queue_init(struct vb2_queue *q) if (q->buf_struct_size == 0) q->buf_struct_size = sizeof(struct vb2_buffer); - return 0; } EXPORT_SYMBOL_GPL(vb2_queue_init); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 8dd9b6c..ed6854a 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -324,7 +324,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req); int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create); int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b); -int vb2_queue_init(struct vb2_queue *q); +void vb2_queue_init(struct vb2_queue *q); void vb2_queue_release(struct vb2_queue *q);
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com> --- drivers/media/v4l2-core/videobuf2-core.c | 3 +-- include/media/videobuf2-core.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)