* [PATCH] media: allegro: Fix use after free on error
@ 2020-12-14 11:54 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-12-14 11:54 UTC (permalink / raw)
To: Michael Tretter
Cc: devel, kernel-janitors, Greg Kroah-Hartman, Chuhong Yuan,
Pengutronix Kernel Team, Hans Verkuil, Mauro Carvalho Chehab,
linux-media
The "channel" is added to the "dev->channels" but then if
v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
list so it could lead to a use after free. Let's not add it to the
list until after v4l2_m2m_ctx_init() succeeds.
Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis. Not tested.
drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 9f718f43282b..640451134072 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
INIT_LIST_HEAD(&channel->buffers_reference);
INIT_LIST_HEAD(&channel->buffers_intermediate);
- list_add(&channel->list, &dev->channels);
-
channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
allegro_queue_init);
@@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
goto error;
}
+ list_add(&channel->list, &dev->channels);
file->private_data = &channel->fh;
v4l2_fh_add(&channel->fh);
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] media: allegro: Fix use after free on error
@ 2020-12-14 11:54 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-12-14 11:54 UTC (permalink / raw)
To: Michael Tretter
Cc: Pengutronix Kernel Team, Mauro Carvalho Chehab,
Greg Kroah-Hartman, Hans Verkuil, Chuhong Yuan, linux-media,
devel, kernel-janitors
The "channel" is added to the "dev->channels" but then if
v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
list so it could lead to a use after free. Let's not add it to the
list until after v4l2_m2m_ctx_init() succeeds.
Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis. Not tested.
drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 9f718f43282b..640451134072 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
INIT_LIST_HEAD(&channel->buffers_reference);
INIT_LIST_HEAD(&channel->buffers_intermediate);
- list_add(&channel->list, &dev->channels);
-
channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
allegro_queue_init);
@@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
goto error;
}
+ list_add(&channel->list, &dev->channels);
file->private_data = &channel->fh;
v4l2_fh_add(&channel->fh);
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] media: allegro: Fix use after free on error
2020-12-14 11:54 ` Dan Carpenter
@ 2020-12-14 17:16 ` Michael Tretter
-1 siblings, 0 replies; 6+ messages in thread
From: Michael Tretter @ 2020-12-14 17:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, kernel-janitors, Greg Kroah-Hartman, Chuhong Yuan,
Pengutronix Kernel Team, Hans Verkuil, Mauro Carvalho Chehab,
linux-media
On Mon, 14 Dec 2020 14:54:47 +0300, Dan Carpenter wrote:
> The "channel" is added to the "dev->channels" but then if
> v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
> list so it could lead to a use after free. Let's not add it to the
> list until after v4l2_m2m_ctx_init() succeeds.
Thanks.
The patch conflicts with the series that moves the driver from staging to
mainline [0]. I'm not sure, which patch should go in first.
It is also correct to not change the order of list_del and
v4l2_m2m_ctx_release in allegro_release. The list is used to relate messages
from the VCU to their destination channel and this should be possible until
the context has been released and no further messages are expected for that
channel.
[0] https://lore.kernel.org/linux-media/20201202133040.1954837-1-m.tretter@pengutronix.de/
>
> Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> From static analysis. Not tested.
>
> drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 9f718f43282b..640451134072 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
> INIT_LIST_HEAD(&channel->buffers_reference);
> INIT_LIST_HEAD(&channel->buffers_intermediate);
>
> - list_add(&channel->list, &dev->channels);
> -
> channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
> allegro_queue_init);
>
> @@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
> goto error;
> }
>
> + list_add(&channel->list, &dev->channels);
> file->private_data = &channel->fh;
> v4l2_fh_add(&channel->fh);
>
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] media: allegro: Fix use after free on error
@ 2020-12-14 17:16 ` Michael Tretter
0 siblings, 0 replies; 6+ messages in thread
From: Michael Tretter @ 2020-12-14 17:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Pengutronix Kernel Team, Mauro Carvalho Chehab,
Greg Kroah-Hartman, Hans Verkuil, Chuhong Yuan, linux-media,
devel, kernel-janitors
On Mon, 14 Dec 2020 14:54:47 +0300, Dan Carpenter wrote:
> The "channel" is added to the "dev->channels" but then if
> v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
> list so it could lead to a use after free. Let's not add it to the
> list until after v4l2_m2m_ctx_init() succeeds.
Thanks.
The patch conflicts with the series that moves the driver from staging to
mainline [0]. I'm not sure, which patch should go in first.
It is also correct to not change the order of list_del and
v4l2_m2m_ctx_release in allegro_release. The list is used to relate messages
from the VCU to their destination channel and this should be possible until
the context has been released and no further messages are expected for that
channel.
[0] https://lore.kernel.org/linux-media/20201202133040.1954837-1-m.tretter@pengutronix.de/
>
> Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> From static analysis. Not tested.
>
> drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 9f718f43282b..640451134072 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
> INIT_LIST_HEAD(&channel->buffers_reference);
> INIT_LIST_HEAD(&channel->buffers_intermediate);
>
> - list_add(&channel->list, &dev->channels);
> -
> channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
> allegro_queue_init);
>
> @@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
> goto error;
> }
>
> + list_add(&channel->list, &dev->channels);
> file->private_data = &channel->fh;
> v4l2_fh_add(&channel->fh);
>
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] media: allegro: Fix use after free on error
2020-12-14 17:16 ` Michael Tretter
@ 2020-12-16 9:38 ` Hans Verkuil
-1 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2020-12-16 9:38 UTC (permalink / raw)
To: Michael Tretter, Dan Carpenter, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Chuhong Yuan,
linux-media, devel, kernel-janitors
On 14/12/2020 18:16, Michael Tretter wrote:
> On Mon, 14 Dec 2020 14:54:47 +0300, Dan Carpenter wrote:
>> The "channel" is added to the "dev->channels" but then if
>> v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
>> list so it could lead to a use after free. Let's not add it to the
>> list until after v4l2_m2m_ctx_init() succeeds.
>
> Thanks.
>
> The patch conflicts with the series that moves the driver from staging to
> mainline [0]. I'm not sure, which patch should go in first.
I'll take care of the conflict.
Regards,
Hans
>
> It is also correct to not change the order of list_del and
> v4l2_m2m_ctx_release in allegro_release. The list is used to relate messages
> from the VCU to their destination channel and this should be possible until
> the context has been released and no further messages are expected for that
> channel.
>
> [0] https://lore.kernel.org/linux-media/20201202133040.1954837-1-m.tretter@pengutronix.de/
>
>>
>> Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>
>
>> ---
>> From static analysis. Not tested.
>>
>> drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
>> index 9f718f43282b..640451134072 100644
>> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
>> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
>> @@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
>> INIT_LIST_HEAD(&channel->buffers_reference);
>> INIT_LIST_HEAD(&channel->buffers_intermediate);
>>
>> - list_add(&channel->list, &dev->channels);
>> -
>> channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
>> allegro_queue_init);
>>
>> @@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
>> goto error;
>> }
>>
>> + list_add(&channel->list, &dev->channels);
>> file->private_data = &channel->fh;
>> v4l2_fh_add(&channel->fh);
>>
>> --
>> 2.29.2
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] media: allegro: Fix use after free on error
@ 2020-12-16 9:38 ` Hans Verkuil
0 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2020-12-16 9:38 UTC (permalink / raw)
To: Michael Tretter, Dan Carpenter, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Chuhong Yuan,
linux-media, devel, kernel-janitors
On 14/12/2020 18:16, Michael Tretter wrote:
> On Mon, 14 Dec 2020 14:54:47 +0300, Dan Carpenter wrote:
>> The "channel" is added to the "dev->channels" but then if
>> v4l2_m2m_ctx_init() fails then we free "channel" but it's still on the
>> list so it could lead to a use after free. Let's not add it to the
>> list until after v4l2_m2m_ctx_init() succeeds.
>
> Thanks.
>
> The patch conflicts with the series that moves the driver from staging to
> mainline [0]. I'm not sure, which patch should go in first.
I'll take care of the conflict.
Regards,
Hans
>
> It is also correct to not change the order of list_del and
> v4l2_m2m_ctx_release in allegro_release. The list is used to relate messages
> from the VCU to their destination channel and this should be possible until
> the context has been released and no further messages are expected for that
> channel.
>
> [0] https://lore.kernel.org/linux-media/20201202133040.1954837-1-m.tretter@pengutronix.de/
>
>>
>> Fixes: cc62c74749a3 ("media: allegro: add missed checks in allegro_open()")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>
>
>> ---
>> From static analysis. Not tested.
>>
>> drivers/staging/media/allegro-dvt/allegro-core.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
>> index 9f718f43282b..640451134072 100644
>> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
>> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
>> @@ -2483,8 +2483,6 @@ static int allegro_open(struct file *file)
>> INIT_LIST_HEAD(&channel->buffers_reference);
>> INIT_LIST_HEAD(&channel->buffers_intermediate);
>>
>> - list_add(&channel->list, &dev->channels);
>> -
>> channel->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, channel,
>> allegro_queue_init);
>>
>> @@ -2493,6 +2491,7 @@ static int allegro_open(struct file *file)
>> goto error;
>> }
>>
>> + list_add(&channel->list, &dev->channels);
>> file->private_data = &channel->fh;
>> v4l2_fh_add(&channel->fh);
>>
>> --
>> 2.29.2
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-16 9:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-14 11:54 [PATCH] media: allegro: Fix use after free on error Dan Carpenter
2020-12-14 11:54 ` Dan Carpenter
2020-12-14 17:16 ` Michael Tretter
2020-12-14 17:16 ` Michael Tretter
2020-12-16 9:38 ` Hans Verkuil
2020-12-16 9:38 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.