Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()

2018-07-18 Thread Hans Verkuil 
On 18/07/18 12:06, Sakari Ailus wrote:
> On Wed, Jul 18, 2018 at 11:29:01AM +0200, Hans Verkuil wrote:
>> On 16/07/18 14:49, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
 The vb2_core_qbuf() function didn't check if q->error was set. It is 
 checked in
 __buf_prepare(), but that function isn't called if the buffer was already
 prepared before with VIDIOC_PREPARE_BUF.

 So check it at the start of vb2_core_qbuf() as well.

 Signed-off-by: Hans Verkuil 
 ---
 diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
 b/drivers/media/common/videobuf2/videobuf2-core.c
 index d3501cd604cb..5d7946ec80d8 100644
 --- a/drivers/media/common/videobuf2/videobuf2-core.c
 +++ b/drivers/media/common/videobuf2/videobuf2-core.c
 @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
 index, void *pb,
struct vb2_buffer *vb;
int ret;

 +  if (q->error) {
 +  dprintk(1, "fatal error occurred on queue\n");
 +  return -EIO;
 +  }
 +
vb = q->bufs[index];

if ((req && q->uses_qbuf) ||
>>>
>>> How long has this problem existed? It looks like something that should go
>>> to the stable branches, too...
>>
>> It's always been there, but I don't think it is worth backporting. The use of
>> VIDIOC_PREPARE_BUF is very rare, let alone the combination with 
>> vb2_queue_error().
>>
>> I came across it while reviewing code.
> 
> What's the effect of the missing check? That the user may queue a buffer
> when the driver thinks the hardware won't be able to complete it? At least
> that doesn't seem like a security issue.

Right. But e.g. dqbuf will still return EIO in this case, so normally apps
will discover this error condition when dequeueing and not when enqueueing
buffers.

> 
> Anyway,
> 
> Acked-by: Sakari Ailus 
> 

Thanks,

Hans

Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()

2018-07-18 Thread Sakari Ailus 
On Wed, Jul 18, 2018 at 11:29:01AM +0200, Hans Verkuil wrote:
> On 16/07/18 14:49, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
> >> The vb2_core_qbuf() function didn't check if q->error was set. It is 
> >> checked in
> >> __buf_prepare(), but that function isn't called if the buffer was already
> >> prepared before with VIDIOC_PREPARE_BUF.
> >>
> >> So check it at the start of vb2_core_qbuf() as well.
> >>
> >> Signed-off-by: Hans Verkuil 
> >> ---
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> >> b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index d3501cd604cb..5d7946ec80d8 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> >> index, void *pb,
> >>struct vb2_buffer *vb;
> >>int ret;
> >>
> >> +  if (q->error) {
> >> +  dprintk(1, "fatal error occurred on queue\n");
> >> +  return -EIO;
> >> +  }
> >> +
> >>vb = q->bufs[index];
> >>
> >>if ((req && q->uses_qbuf) ||
> > 
> > How long has this problem existed? It looks like something that should go
> > to the stable branches, too...
> 
> It's always been there, but I don't think it is worth backporting. The use of
> VIDIOC_PREPARE_BUF is very rare, let alone the combination with 
> vb2_queue_error().
> 
> I came across it while reviewing code.

What's the effect of the missing check? That the user may queue a buffer
when the driver thinks the hardware won't be able to complete it? At least
that doesn't seem like a security issue.

Anyway,

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()

2018-07-18 Thread Hans Verkuil 
On 16/07/18 14:49, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
>> The vb2_core_qbuf() function didn't check if q->error was set. It is checked 
>> in
>> __buf_prepare(), but that function isn't called if the buffer was already
>> prepared before with VIDIOC_PREPARE_BUF.
>>
>> So check it at the start of vb2_core_qbuf() as well.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index d3501cd604cb..5d7946ec80d8 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb,
>>  struct vb2_buffer *vb;
>>  int ret;
>>
>> +if (q->error) {
>> +dprintk(1, "fatal error occurred on queue\n");
>> +return -EIO;
>> +}
>> +
>>  vb = q->bufs[index];
>>
>>  if ((req && q->uses_qbuf) ||
> 
> How long has this problem existed? It looks like something that should go
> to the stable branches, too...

It's always been there, but I don't think it is worth backporting. The use of
VIDIOC_PREPARE_BUF is very rare, let alone the combination with 
vb2_queue_error().

I came across it while reviewing code.

Regards,

Hans

Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()

2018-07-16 Thread Sakari Ailus 
Hi Hans,

On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
> The vb2_core_qbuf() function didn't check if q->error was set. It is checked 
> in
> __buf_prepare(), but that function isn't called if the buffer was already
> prepared before with VIDIOC_PREPARE_BUF.
> 
> So check it at the start of vb2_core_qbuf() as well.
> 
> Signed-off-by: Hans Verkuil 
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index d3501cd604cb..5d7946ec80d8 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb,
>   struct vb2_buffer *vb;
>   int ret;
> 
> + if (q->error) {
> + dprintk(1, "fatal error occurred on queue\n");
> + return -EIO;
> + }
> +
>   vb = q->bufs[index];
> 
>   if ((req && q->uses_qbuf) ||

How long has this problem existed? It looks like something that should go
to the stable branches, too...

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

[PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()

2018-07-05 Thread Hans Verkuil 
The vb2_core_qbuf() function didn't check if q->error was set. It is checked in
__buf_prepare(), but that function isn't called if the buffer was already
prepared before with VIDIOC_PREPARE_BUF.

So check it at the start of vb2_core_qbuf() as well.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index d3501cd604cb..5d7946ec80d8 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb,
struct vb2_buffer *vb;
int ret;

+   if (q->error) {
+   dprintk(1, "fatal error occurred on queue\n");
+   return -EIO;
+   }
+
vb = q->bufs[index];

if ((req && q->uses_qbuf) ||