diff mbox series

[v2] media: dvb_vb2: fix possible out of bound access

Message ID 20220519021743.8295-1-hbh25y@gmail.com (mailing list archive)
State Accepted
Delegated to: Hans Verkuil
Headers
Series [v2] media: dvb_vb2: fix possible out of bound access |

Commit Message

Hangyu Hua May 19, 2022, 2:17 a.m. UTC 
  vb2_core_qbuf and vb2_core_querybuf don't check the range of b->index
controlled by the user.

Fix this by adding range checking code before using them.

Fixes: 57868acc369a ("media: videobuf2: Add new uAPI for DVB streaming I/O")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---

v2: 
1. fix inappropriate use of dprintk.
2. add "fixes" tag

 drivers/media/dvb-core/dvb_vb2.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Hangyu Hua Sept. 27, 2022, 2:01 a.m. UTC | #1 
On 19/5/2022 10:17, Hangyu Hua wrote:
> vb2_core_qbuf and vb2_core_querybuf don't check the range of b->index
> controlled by the user.
> 
> Fix this by adding range checking code before using them.
> 
> Fixes: 57868acc369a ("media: videobuf2: Add new uAPI for DVB streaming I/O")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
> 
> v2:
> 1. fix inappropriate use of dprintk.
> 2. add "fixes" tag
> 
>   drivers/media/dvb-core/dvb_vb2.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
> index a1bd6d9c9223..909df82fed33 100644
> --- a/drivers/media/dvb-core/dvb_vb2.c
> +++ b/drivers/media/dvb-core/dvb_vb2.c
> @@ -354,6 +354,12 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req)
>   
>   int dvb_vb2_querybuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>   {
> +	struct vb2_queue *q = &ctx->vb_q;
> +
> +	if (b->index >= q->num_buffers) {
> +		dprintk(1, "[%s] buffer index out of range\n", ctx->name);
> +		return -EINVAL;
> +	}
>   	vb2_core_querybuf(&ctx->vb_q, b->index, b);
>   	dprintk(3, "[%s] index=%d\n", ctx->name, b->index);
>   	return 0;
> @@ -378,8 +384,13 @@ int dvb_vb2_expbuf(struct dvb_vb2_ctx *ctx, struct dmx_exportbuffer *exp)
>   
>   int dvb_vb2_qbuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>   {
> +	struct vb2_queue *q = &ctx->vb_q;
>   	int ret;
>   
> +	if (b->index >= q->num_buffers) {
> +		dprintk(1, "[%s] buffer index out of range\n", ctx->name);
> +		return -EINVAL;
> +	}
>   	ret = vb2_core_qbuf(&ctx->vb_q, b->index, b, NULL);
>   	if (ret) {
>   		dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,

Hi guys,

Looks like this patch was forgotten to to merge into master branch. This 
bug still in:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n355
and
https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n379

Thanks,
Hangyu
Hans Verkuil Sept. 27, 2022, 8:10 a.m. UTC | #2 
On 27/09/2022 04:01, Hangyu Hua wrote:
> On 19/5/2022 10:17, Hangyu Hua wrote:
>> vb2_core_qbuf and vb2_core_querybuf don't check the range of b->index
>> controlled by the user.
>>
>> Fix this by adding range checking code before using them.
>>
>> Fixes: 57868acc369a ("media: videobuf2: Add new uAPI for DVB streaming I/O")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>> ---
>>
>> v2:
>> 1. fix inappropriate use of dprintk.
>> 2. add "fixes" tag
>>
>>   drivers/media/dvb-core/dvb_vb2.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
>> index a1bd6d9c9223..909df82fed33 100644
>> --- a/drivers/media/dvb-core/dvb_vb2.c
>> +++ b/drivers/media/dvb-core/dvb_vb2.c
>> @@ -354,6 +354,12 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req)
>>     int dvb_vb2_querybuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>>   {
>> +    struct vb2_queue *q = &ctx->vb_q;
>> +
>> +    if (b->index >= q->num_buffers) {
>> +        dprintk(1, "[%s] buffer index out of range\n", ctx->name);
>> +        return -EINVAL;
>> +    }
>>       vb2_core_querybuf(&ctx->vb_q, b->index, b);
>>       dprintk(3, "[%s] index=%d\n", ctx->name, b->index);
>>       return 0;
>> @@ -378,8 +384,13 @@ int dvb_vb2_expbuf(struct dvb_vb2_ctx *ctx, struct dmx_exportbuffer *exp)
>>     int dvb_vb2_qbuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>>   {
>> +    struct vb2_queue *q = &ctx->vb_q;
>>       int ret;
>>   +    if (b->index >= q->num_buffers) {
>> +        dprintk(1, "[%s] buffer index out of range\n", ctx->name);
>> +        return -EINVAL;
>> +    }
>>       ret = vb2_core_qbuf(&ctx->vb_q, b->index, b, NULL);
>>       if (ret) {
>>           dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,
> 
> Hi guys,
> 
> Looks like this patch was forgotten to to merge into master branch. This bug still in:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n355
> and
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n379
> 
> Thanks,
> Hangyu

That's weird, it was part of this pull request:

https://patchwork.linuxtv.org/project/linux-media/patch/2eeaad13-091d-6547-cdeb-0a7a15dc5c3f@xs4all.nl/

But none of the patches in that PR ever made it to upstream. Something went very wrong
with that PR.

I'm preparing a new pull request.

Thank you very much for notifying me!

Regards,

	Hans
Hans Verkuil Sept. 27, 2022, 8:36 a.m. UTC | #3 
Hi Hangyu,

On 27/09/2022 10:10, Hans Verkuil wrote:
> On 27/09/2022 04:01, Hangyu Hua wrote:
>> On 19/5/2022 10:17, Hangyu Hua wrote:
>>> vb2_core_qbuf and vb2_core_querybuf don't check the range of b->index
>>> controlled by the user.
>>>
>>> Fix this by adding range checking code before using them.
>>>
>>> Fixes: 57868acc369a ("media: videobuf2: Add new uAPI for DVB streaming I/O")
>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>>> ---
>>>
>>> v2:
>>> 1. fix inappropriate use of dprintk.
>>> 2. add "fixes" tag
>>>
>>>   drivers/media/dvb-core/dvb_vb2.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
>>> index a1bd6d9c9223..909df82fed33 100644
>>> --- a/drivers/media/dvb-core/dvb_vb2.c
>>> +++ b/drivers/media/dvb-core/dvb_vb2.c
>>> @@ -354,6 +354,12 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req)
>>>     int dvb_vb2_querybuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>>>   {
>>> +    struct vb2_queue *q = &ctx->vb_q;
>>> +
>>> +    if (b->index >= q->num_buffers) {
>>> +        dprintk(1, "[%s] buffer index out of range\n", ctx->name);
>>> +        return -EINVAL;
>>> +    }
>>>       vb2_core_querybuf(&ctx->vb_q, b->index, b);
>>>       dprintk(3, "[%s] index=%d\n", ctx->name, b->index);
>>>       return 0;
>>> @@ -378,8 +384,13 @@ int dvb_vb2_expbuf(struct dvb_vb2_ctx *ctx, struct dmx_exportbuffer *exp)
>>>     int dvb_vb2_qbuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
>>>   {
>>> +    struct vb2_queue *q = &ctx->vb_q;
>>>       int ret;
>>>   +    if (b->index >= q->num_buffers) {
>>> +        dprintk(1, "[%s] buffer index out of range\n", ctx->name);
>>> +        return -EINVAL;
>>> +    }
>>>       ret = vb2_core_qbuf(&ctx->vb_q, b->index, b, NULL);
>>>       if (ret) {
>>>           dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,
>>
>> Hi guys,
>>
>> Looks like this patch was forgotten to to merge into master branch. This bug still in:
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n355
>> and
>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/dvb-core/dvb_vb2.c#n379
>>
>> Thanks,
>> Hangyu
> 
> That's weird, it was part of this pull request:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/2eeaad13-091d-6547-cdeb-0a7a15dc5c3f@xs4all.nl/
> 
> But none of the patches in that PR ever made it to upstream. Something went very wrong
> with that PR.
> 
> I'm preparing a new pull request.
> 
> Thank you very much for notifying me!

Mauro discovered that a small number of patches were never pushed to our git trees,
and so indeed got lost. They have now been merged. This patch has been really unlucky:
it took a long time for the original patch to be reviewed, and then the PR itself
fell through the cracks :-(

Again, thank you very much for double checking this!

Regards,

	Hans
diff mbox series

Patch

diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
index a1bd6d9c9223..909df82fed33 100644
--- a/drivers/media/dvb-core/dvb_vb2.c
+++ b/drivers/media/dvb-core/dvb_vb2.c
@@ -354,6 +354,12 @@  int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req)
 
 int dvb_vb2_querybuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
 {
+	struct vb2_queue *q = &ctx->vb_q;
+
+	if (b->index >= q->num_buffers) {
+		dprintk(1, "[%s] buffer index out of range\n", ctx->name);
+		return -EINVAL;
+	}
 	vb2_core_querybuf(&ctx->vb_q, b->index, b);
 	dprintk(3, "[%s] index=%d\n", ctx->name, b->index);
 	return 0;
@@ -378,8 +384,13 @@  int dvb_vb2_expbuf(struct dvb_vb2_ctx *ctx, struct dmx_exportbuffer *exp)
 
 int dvb_vb2_qbuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
 {
+	struct vb2_queue *q = &ctx->vb_q;
 	int ret;
 
+	if (b->index >= q->num_buffers) {
+		dprintk(1, "[%s] buffer index out of range\n", ctx->name);
+		return -EINVAL;
+	}
 	ret = vb2_core_qbuf(&ctx->vb_q, b->index, b, NULL);
 	if (ret) {
 		dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,