* [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small @ 2011-08-04 15:40 Michael Jones 2011-08-05 8:59 ` Laurent Pinchart 2011-08-09 6:42 ` [PATCH v2] [media] omap3isp: queue: fail QBUF if user " Michael Jones 0 siblings, 2 replies; 7+ messages in thread From: Michael Jones @ 2011-08-04 15:40 UTC (permalink / raw) To: linux-media; +Cc: Laurent Pinchart, Mauro Carvalho Chehab Add buffer length to sanity checks for QBUF. Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> --- drivers/media/video/omap3isp/ispqueue.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/omap3isp/ispqueue.c b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..4f6876f 100644 --- a/drivers/media/video/omap3isp/ispqueue.c +++ b/drivers/media/video/omap3isp/ispqueue.c @@ -867,6 +867,9 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue *queue, if (buf->state != ISP_BUF_STATE_IDLE) goto done; + if (vbuf->length < buf->vbuf.length) + goto done; + if (vbuf->memory == V4L2_MEMORY_USERPTR && vbuf->m.userptr != buf->vbuf.m.userptr) { isp_video_buffer_cleanup(buf); -- 1.7.6 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small 2011-08-04 15:40 [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small Michael Jones @ 2011-08-05 8:59 ` Laurent Pinchart 2011-08-05 11:41 ` Michael Jones 2011-08-09 6:42 ` [PATCH v2] [media] omap3isp: queue: fail QBUF if user " Michael Jones 1 sibling, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2011-08-05 8:59 UTC (permalink / raw) To: Michael Jones; +Cc: linux-media, Mauro Carvalho Chehab Hi Michael, Thanks for the patch. On Thursday 04 August 2011 17:40:37 Michael Jones wrote: > Add buffer length to sanity checks for QBUF. > > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > --- > drivers/media/video/omap3isp/ispqueue.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/omap3isp/ispqueue.c > b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..4f6876f 100644 > --- a/drivers/media/video/omap3isp/ispqueue.c > +++ b/drivers/media/video/omap3isp/ispqueue.c > @@ -867,6 +867,9 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue > *queue, if (buf->state != ISP_BUF_STATE_IDLE) > goto done; > > + if (vbuf->length < buf->vbuf.length) > + goto done; > + The vbuf->length value passed from userspace isn't used by the driver, so I'm not sure if verifying it is really useful. We verify the memory itself instead, to make sure that enough pages can be accessed. The application can always lie about the length, so we can't relying on it anyway. > if (vbuf->memory == V4L2_MEMORY_USERPTR && > vbuf->m.userptr != buf->vbuf.m.userptr) { > isp_video_buffer_cleanup(buf); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small 2011-08-05 8:59 ` Laurent Pinchart @ 2011-08-05 11:41 ` Michael Jones 2011-08-08 10:08 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Michael Jones @ 2011-08-05 11:41 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Mauro Carvalho Chehab Hi Laurent, On 08/05/2011 10:59 AM, Laurent Pinchart wrote: > > Hi Michael, > > Thanks for the patch. > > On Thursday 04 August 2011 17:40:37 Michael Jones wrote: >> Add buffer length to sanity checks for QBUF. >> >> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> >> --- >> drivers/media/video/omap3isp/ispqueue.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/media/video/omap3isp/ispqueue.c >> b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..4f6876f 100644 >> --- a/drivers/media/video/omap3isp/ispqueue.c >> +++ b/drivers/media/video/omap3isp/ispqueue.c >> @@ -867,6 +867,9 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue >> *queue, if (buf->state != ISP_BUF_STATE_IDLE) >> goto done; >> >> + if (vbuf->length < buf->vbuf.length) >> + goto done; >> + > > The vbuf->length value passed from userspace isn't used by the driver, so I'm > not sure if verifying it is really useful. We verify the memory itself > instead, to make sure that enough pages can be accessed. The application can > always lie about the length, so we can't rely on it anyway. According to the spec, it's expected that the application set 'length': "To enqueue a user pointer buffer applications set [...] length to its size." (Now that I say that, I realize I should only do this length check for USERPTR buffers.) If we don't at least sanity check it for the application, then it has no purpose at all on QBUF. If this is desirable, I would propose changing the spec. This patch was born of a mistake when my application set 624x480, which resulted in sizeimage=640x480=307200 but it used width & height to calculate the buffer size rather than sizeimage or even to take bytesperline into account. It was then honest with QBUF, confessing that it wasn't providing enough space, but QBUF just went ahead. What followed were random crashes while data was DMA'd into memory not set aside for the buffer, while I assumed that the buffer size was OK because QBUF had succeeded and was looking elsewhere in the program for the culprit. I think it makes sense to give the app an error on QBUF in this situation. > >> if (vbuf->memory == V4L2_MEMORY_USERPTR && >> vbuf->m.userptr != buf->vbuf.m.userptr) { >> isp_video_buffer_cleanup(buf); > MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small 2011-08-05 11:41 ` Michael Jones @ 2011-08-08 10:08 ` Laurent Pinchart 2011-08-08 10:16 ` Michael Jones 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2011-08-08 10:08 UTC (permalink / raw) To: Michael Jones; +Cc: linux-media, Mauro Carvalho Chehab Hi Michael, On Friday 05 August 2011 13:41:54 Michael Jones wrote: > On 08/05/2011 10:59 AM, Laurent Pinchart wrote: > > Hi Michael, > > > > Thanks for the patch. > > > > On Thursday 04 August 2011 17:40:37 Michael Jones wrote: > >> Add buffer length to sanity checks for QBUF. > >> > >> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> > >> --- > >> > >> drivers/media/video/omap3isp/ispqueue.c | 3 +++ > >> 1 files changed, 3 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/media/video/omap3isp/ispqueue.c > >> b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..4f6876f 100644 > >> --- a/drivers/media/video/omap3isp/ispqueue.c > >> +++ b/drivers/media/video/omap3isp/ispqueue.c > >> @@ -867,6 +867,9 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue > >> *queue, if (buf->state != ISP_BUF_STATE_IDLE) > >> > >> goto done; > >> > >> + if (vbuf->length < buf->vbuf.length) > >> + goto done; > >> + > > > > The vbuf->length value passed from userspace isn't used by the driver, so > > I'm not sure if verifying it is really useful. We verify the memory > > itself instead, to make sure that enough pages can be accessed. The > > application can always lie about the length, so we can't rely on it > > anyway. > > According to the spec, it's expected that the application set 'length': > "To enqueue a user pointer buffer applications set [...] length to its > size." (Now that I say that, I realize I should only do this length > check for USERPTR buffers.) If we don't at least sanity check it for the > application, then it has no purpose at all on QBUF. If this is > desirable, I would propose changing the spec. > > This patch was born of a mistake when my application set 624x480, which > resulted in sizeimage=640x480=307200 but it used width & height to > calculate the buffer size rather than sizeimage or even to take > bytesperline into account. It was then honest with QBUF, confessing that > it wasn't providing enough space, but QBUF just went ahead. What > followed were random crashes while data was DMA'd into memory not set > aside for the buffer, while I assumed that the buffer size was OK > because QBUF had succeeded and was looking elsewhere in the program for > the culprit. I think it makes sense to give the app an error on QBUF in > this situation. Right. This will help catching application errors without any drawback on the kernel side. Do you want to resubmit the patch with an additional USERPTR check, or should I write one ? > >> if (vbuf->memory == V4L2_MEMORY_USERPTR && > >> vbuf->m.userptr != buf->vbuf.m.userptr) { > >> isp_video_buffer_cleanup(buf); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small 2011-08-08 10:08 ` Laurent Pinchart @ 2011-08-08 10:16 ` Michael Jones 0 siblings, 0 replies; 7+ messages in thread From: Michael Jones @ 2011-08-08 10:16 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Mauro Carvalho Chehab Hi Laurent, On 08/08/2011 12:08 PM, Laurent Pinchart wrote: > > Hi Michael, > > On Friday 05 August 2011 13:41:54 Michael Jones wrote: >> On 08/05/2011 10:59 AM, Laurent Pinchart wrote: >>> Hi Michael, >>> >>> Thanks for the patch. >>> >>> On Thursday 04 August 2011 17:40:37 Michael Jones wrote: >>>> Add buffer length to sanity checks for QBUF. >>>> >>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> >>>> --- >>>> >>>> drivers/media/video/omap3isp/ispqueue.c | 3 +++ >>>> 1 files changed, 3 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/media/video/omap3isp/ispqueue.c >>>> b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..4f6876f 100644 >>>> --- a/drivers/media/video/omap3isp/ispqueue.c >>>> +++ b/drivers/media/video/omap3isp/ispqueue.c >>>> @@ -867,6 +867,9 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue >>>> *queue, if (buf->state != ISP_BUF_STATE_IDLE) >>>> >>>> goto done; >>>> >>>> + if (vbuf->length < buf->vbuf.length) >>>> + goto done; >>>> + >>> >>> The vbuf->length value passed from userspace isn't used by the driver, so >>> I'm not sure if verifying it is really useful. We verify the memory >>> itself instead, to make sure that enough pages can be accessed. The >>> application can always lie about the length, so we can't rely on it >>> anyway. >> >> According to the spec, it's expected that the application set 'length': >> "To enqueue a user pointer buffer applications set [...] length to its >> size." (Now that I say that, I realize I should only do this length >> check for USERPTR buffers.) If we don't at least sanity check it for the >> application, then it has no purpose at all on QBUF. If this is >> desirable, I would propose changing the spec. >> >> This patch was born of a mistake when my application set 624x480, which >> resulted in sizeimage=640x480=307200 but it used width & height to >> calculate the buffer size rather than sizeimage or even to take >> bytesperline into account. It was then honest with QBUF, confessing that >> it wasn't providing enough space, but QBUF just went ahead. What >> followed were random crashes while data was DMA'd into memory not set >> aside for the buffer, while I assumed that the buffer size was OK >> because QBUF had succeeded and was looking elsewhere in the program for >> the culprit. I think it makes sense to give the app an error on QBUF in >> this situation. > > Right. This will help catching application errors without any drawback on the > kernel side. > > Do you want to resubmit the patch with an additional USERPTR check, or should > I write one ? Thanks for the review. I will resubmit the patch with the USERPTR check. > >>>> if (vbuf->memory == V4L2_MEMORY_USERPTR && >>>> vbuf->m.userptr != buf->vbuf.m.userptr) { >>>> isp_video_buffer_cleanup(buf); > MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] [media] omap3isp: queue: fail QBUF if user buffer is too small 2011-08-04 15:40 [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small Michael Jones 2011-08-05 8:59 ` Laurent Pinchart @ 2011-08-09 6:42 ` Michael Jones 2011-08-09 7:42 ` Laurent Pinchart 1 sibling, 1 reply; 7+ messages in thread From: Michael Jones @ 2011-08-09 6:42 UTC (permalink / raw) To: linux-media; +Cc: Laurent Pinchart, Mauro Carvalho Chehab Add buffer length check to sanity checks for USERPTR QBUF Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> --- Changes for v2: - only check when V4L2_MEMORY_USERPTR drivers/media/video/omap3isp/ispqueue.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/omap3isp/ispqueue.c b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..9bebb1e 100644 --- a/drivers/media/video/omap3isp/ispqueue.c +++ b/drivers/media/video/omap3isp/ispqueue.c @@ -868,6 +868,10 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue *queue, goto done; if (vbuf->memory == V4L2_MEMORY_USERPTR && + vbuf->length < buf->vbuf.length) + goto done; + + if (vbuf->memory == V4L2_MEMORY_USERPTR && vbuf->m.userptr != buf->vbuf.m.userptr) { isp_video_buffer_cleanup(buf); buf->vbuf.m.userptr = vbuf->m.userptr; -- 1.7.6 MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] [media] omap3isp: queue: fail QBUF if user buffer is too small 2011-08-09 6:42 ` [PATCH v2] [media] omap3isp: queue: fail QBUF if user " Michael Jones @ 2011-08-09 7:42 ` Laurent Pinchart 0 siblings, 0 replies; 7+ messages in thread From: Laurent Pinchart @ 2011-08-09 7:42 UTC (permalink / raw) To: Michael Jones; +Cc: linux-media, Mauro Carvalho Chehab Hi Michael, On Tuesday 09 August 2011 08:42:20 Michael Jones wrote: > Add buffer length check to sanity checks for USERPTR QBUF > > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'll push the patch to v3.2. > --- > Changes for v2: > - only check when V4L2_MEMORY_USERPTR > > drivers/media/video/omap3isp/ispqueue.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/omap3isp/ispqueue.c > b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..9bebb1e 100644 > --- a/drivers/media/video/omap3isp/ispqueue.c > +++ b/drivers/media/video/omap3isp/ispqueue.c > @@ -868,6 +868,10 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue > *queue, goto done; > > if (vbuf->memory == V4L2_MEMORY_USERPTR && > + vbuf->length < buf->vbuf.length) > + goto done; > + > + if (vbuf->memory == V4L2_MEMORY_USERPTR && > vbuf->m.userptr != buf->vbuf.m.userptr) { > isp_video_buffer_cleanup(buf); > buf->vbuf.m.userptr = vbuf->m.userptr; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-08-09 7:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-08-04 15:40 [PATCH] [media] omap3isp: queue: fail QBUF if buffer is too small Michael Jones 2011-08-05 8:59 ` Laurent Pinchart 2011-08-05 11:41 ` Michael Jones 2011-08-08 10:08 ` Laurent Pinchart 2011-08-08 10:16 ` Michael Jones 2011-08-09 6:42 ` [PATCH v2] [media] omap3isp: queue: fail QBUF if user " Michael Jones 2011-08-09 7:42 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox