Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/16/21 1:33 PM, Igor Torrente wrote: On 4/15/21 2:25 PM, Shuah Khan wrote: On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h | 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. I tried and everything worked fine. Thank you. Reviewed-by: Shuah Khan thanks, -- Shuah
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/15/21 2:25 PM, Shuah Khan wrote: On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h | 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. I tried and everything worked fine. thanks, -- Shuah Thanks, --- Igor Matheus Andrade Torrente
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h| 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. thanks, -- Shuah
[PATCH RFC v3] media: em28xx: Fix race condition between open and init function
Fixes a race condition - for lack of a more precise term - between
em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
media_pad and vdev structs from the em28xx_v4l2, and managing the
lifetime of those objects more dynamicaly.
The race happens when a thread[1] - containing the em28xx_v4l2_init()
code - calls the v4l2_mc_create_media_graph(), and it return a error,
if a thread[2] - running v4l2_open() - pass the verification point
and reaches the em28xx_v4l2_open() before the thread[1] finishes
the deregistration of v4l2 subsystem, the thread[1] will free all
resources before the em28xx_v4l2_open() can process their things,
because the em28xx_v4l2_init() has the dev->lock. And all this lead
the thread[2] to cause a user-after-free.
Reported-by: kernel test robot
Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com
Signed-off-by: Igor Matheus Andrade Torrente
---
V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies
V3: Fix link error when compiled as a module
---
drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
drivers/media/usb/em28xx/em28xx-video.c | 300 +++
drivers/media/usb/em28xx/em28xx.h| 6 +-
3 files changed, 209 insertions(+), 101 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
b/drivers/media/usb/em28xx/em28xx-camera.c
index d1e66b503f4d..436c5a8cbbb6 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_xtal = 430;
pdata.xtal = v4l2->sensor_xtal;
if (NULL ==
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&mt9v011_info, NULL))
return -ENODEV;
v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
@@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_yres = 480;
subdev =
-v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&ov2640_info, NULL);
if (!subdev)
return -ENODEV;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c
b/drivers/media/usb/em28xx/em28xx-video.c
index 6b84c3413e83..85dfd80fddfb 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
*/
static void em28xx_wake_i2c(struct em28xx *dev)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
@@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int ret, i;
+ v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
+ if (!v4l2->video_pad) {
+ dev_err(&dev->intf->dev,
+ "failed to allocate video pad memory!\n");
+ v4l2->vdev->entity.num_pads = 0;
+ return;
+ }
+
/* Initialize Video, VBI and Radio pads */
- v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
- ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
+ v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
if (ret < 0)
dev_err(&dev->intf->dev,
"failed to initialize video media entity!\n");
@@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq,
unsigned int count)
f.type = V4L2_TUNER_RADIO;
else
f.type = V4L2_TUNER_ANALOG_TV;
- v4l2_device_call_all(&v4l2->v4l2_dev,
+ v4l2_device_call_all(v4l2->v4l2_dev,
0, tuner, s_frequency, &f);
/* Enable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
}
v4l2->streaming_users++;
@@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev
