* [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated
@ 2018-11-12 0:49 ` Myungho Jung
2018-11-13 10:27 ` Marek Szyprowski
0 siblings, 1 reply; 4+ messages in thread
From: Myungho Jung @ 2018-11-12 0:49 UTC (permalink / raw)
To: pawel, m.szyprowski, kyungmin.park, mchehab; +Cc: linux-media, linux-kernel
The mutex that is held from vb2_fop_read() can be unlocked while waiting
for a buffer if the queue is streaming and blocking. Meanwhile, fileio
can be released. So, it should return an error if the fileio address is
changed.
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 975ff5669f72..bff94752eb27 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2564,6 +2564,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
dprintk(5, "vb2_dqbuf result: %d\n", ret);
if (ret)
return ret;
+ if (fileio != q->fileio) {
+ dprintk(3, "fileio deallocated\n");
+ return -EFAULT;
+ }
fileio->dq_count += 1;
fileio->cur_index = index;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated
2018-11-12 0:49 ` [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated Myungho Jung
@ 2018-11-13 10:27 ` Marek Szyprowski
2018-11-13 11:56 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Marek Szyprowski @ 2018-11-13 10:27 UTC (permalink / raw)
To: Myungho Jung, pawel, kyungmin.park, mchehab; +Cc: linux-media, linux-kernel
Hi Myungho,
On 2018-11-12 01:49, Myungho Jung wrote:
> The mutex that is held from vb2_fop_read() can be unlocked while waiting
> for a buffer if the queue is streaming and blocking. Meanwhile, fileio
> can be released. So, it should return an error if the fileio address is
> changed.
>
> Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Thanks for analyzing the code and fixing this issue!
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 975ff5669f72..bff94752eb27 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2564,6 +2564,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> dprintk(5, "vb2_dqbuf result: %d\n", ret);
> if (ret)
> return ret;
> + if (fileio != q->fileio) {
> + dprintk(3, "fileio deallocated\n");
> + return -EFAULT;
> + }
> fileio->dq_count += 1;
>
> fileio->cur_index = index;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated
2018-11-13 10:27 ` Marek Szyprowski
@ 2018-11-13 11:56 ` Hans Verkuil
2018-11-13 17:38 ` Myungho Jung
0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2018-11-13 11:56 UTC (permalink / raw)
To: Marek Szyprowski, Myungho Jung, pawel, kyungmin.park, mchehab
Cc: linux-media, linux-kernel
On 11/13/18 11:27, Marek Szyprowski wrote:
> Hi Myungho,
>
> On 2018-11-12 01:49, Myungho Jung wrote:
>> The mutex that is held from vb2_fop_read() can be unlocked while waiting
>> for a buffer if the queue is streaming and blocking. Meanwhile, fileio
>> can be released. So, it should return an error if the fileio address is
>> changed.
>>
>> Signed-off-by: Myungho Jung <mhjungk@gmail.com>
>> Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Sorry:
Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
This addresses the symptom, not the underlying cause.
I have a patch that fixes the actual cause that I plan to post soon
after I review it a bit more.
Regards,
Hans
>
> Thanks for analyzing the code and fixing this issue!
>
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 975ff5669f72..bff94752eb27 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -2564,6 +2564,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>> dprintk(5, "vb2_dqbuf result: %d\n", ret);
>> if (ret)
>> return ret;
>> + if (fileio != q->fileio) {
>> + dprintk(3, "fileio deallocated\n");
>> + return -EFAULT;
>> + }
>> fileio->dq_count += 1;
>>
>> fileio->cur_index = index;
>
> Best regards
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated
2018-11-13 11:56 ` Hans Verkuil
@ 2018-11-13 17:38 ` Myungho Jung
0 siblings, 0 replies; 4+ messages in thread
From: Myungho Jung @ 2018-11-13 17:38 UTC (permalink / raw)
To: Hans Verkuil, Marek Szyprowski, pawel, kyungmin.park, mchehab
Cc: linux-media, linux-kernel
On Tue, Nov 13, 2018 at 12:56:18PM +0100, Hans Verkuil wrote:
> On 11/13/18 11:27, Marek Szyprowski wrote:
> > Hi Myungho,
> >
> > On 2018-11-12 01:49, Myungho Jung wrote:
> >> The mutex that is held from vb2_fop_read() can be unlocked while waiting
> >> for a buffer if the queue is streaming and blocking. Meanwhile, fileio
> >> can be released. So, it should return an error if the fileio address is
> >> changed.
> >>
> >> Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> >> Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
> >
> > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Sorry:
>
> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> This addresses the symptom, not the underlying cause.
>
> I have a patch that fixes the actual cause that I plan to post soon
> after I review it a bit more.
>
> Regards,
>
> Hans
>
Hi Hans,
Thanks for explaining the root cause. I was also thinking a similar
patch with your second one. It looks like the reported syzbot is needed
to be added to your first patch.
Regards,
Myungho
> >
> > Thanks for analyzing the code and fixing this issue!
> >
> >> ---
> >> drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 975ff5669f72..bff94752eb27 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -2564,6 +2564,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> >> dprintk(5, "vb2_dqbuf result: %d\n", ret);
> >> if (ret)
> >> return ret;
> >> + if (fileio != q->fileio) {
> >> + dprintk(3, "fileio deallocated\n");
> >> + return -EFAULT;
> >> + }
> >> fileio->dq_count += 1;
> >>
> >> fileio->cur_index = index;
> >
> > Best regards
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-13 17:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20181112005053epcas4p1c674759797b4a930cfcce3abc7edd9ad@epcas4p1.samsung.com>
2018-11-12 0:49 ` [PATCH] media: videobuf2-core: Fix error handling when fileio is deallocated Myungho Jung
2018-11-13 10:27 ` Marek Szyprowski
2018-11-13 11:56 ` Hans Verkuil
2018-11-13 17:38 ` Myungho Jung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).