Skip to contentSkip to conversation

media: em28xx: Fix race condition between open and init function

RFC PATCH Superseded Igor Matheus Andrade Torrente 2 participants 4 messages Apr 15, 2021 – Apr 16, 2021

Subsystems (2)
Media (V4L/DVB) EM28XX VIDEO4LINUX driver

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Igor Matheus Andrade Torrente

author
#0 lore raw

RFC PATCH v3 1/1 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 <[email protected]>
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

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(-)
drivers/media/usb/em28xx/em28xx-camera.c +2 -2
index d1e66b503f4d..436c5a8cbbb6 100644
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
 		v4l2->sensor_xtal = 4300000;
 		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;
drivers/media/usb/em28xx/em28xx-video.c +204 -96

581 lines not on this page · raw message

drivers/media/usb/em28xx/em28xx.h +3 -3
index 6648e11f1271..dbcc297b5a0d 100644
@@ -552,10 +552,10 @@ struct em28xx_v4l2 {
 	struct kref ref;
 	struct em28xx *dev;
 
-	struct v4l2_device v4l2_dev;
+	struct v4l2_device *v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 
-	struct video_device vdev;
+	struct video_device *vdev;
 	struct video_device vbi_dev;
 	struct video_device radio_dev;
 

@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
 	unsigned int field_count;
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-	struct media_pad video_pad, vbi_pad;
+	struct media_pad *video_pad, vbi_pad;
 	struct media_entity *decoder;
 #endif
 };
Signature · 1 line
2.20.1
#1 lore raw
Quoted 22 lines · Igor Matheus Andrade Torrente

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 <[email protected]>
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies

Quoted 8 lines

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

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#2 lore raw
Quoted 32 lines · Shuah Khan
Quoted 22 lines · Igor Matheus Andrade Torrente

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 <[email protected]>
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
     Deal with v4l2 subdevs dependencies

Quoted 8 lines

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(-)
Quoted 3 lines

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.

Quoted 3 lines

thanks,
-- Shuah

Thanks,
---
Igor Matheus Andrade Torrente

Reviewed-by
#3 lore raw
Quoted 35 lines · Igor Torrente
Quoted 32 lines · Shuah Khan
Quoted 30 lines · Igor Matheus Andrade Torrente

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 <[email protected]>
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

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(-)
Quoted 6 lines
Quoted 3 lines

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 <[email protected]>

thanks,
-- Shuah