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