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 <[email protected]>
> >> ---
> >>
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
