Messages in this thread Patch in this message | | | Subject | Re: [PATCH v6] media: em28xx: Fix race condition between open and init function | From | Igor Matheus Andrade Torrente <> | Date | Mon, 13 Sep 2021 12:11:54 -0300 |
| |
Hi Hans,
On 9/13/21 6:35 AM, Hans Verkuil wrote: > On 14/06/2021 21:52, 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 managing the em28xx_v4l2 >> and v4l2_dev life-time with the v4l2_dev->release() callback. >> >> 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 <lkp@intel.com> >> Reported-and-tested-by: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com >> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com> >> --- >> >> V2: Add v4l2_i2c_new_subdev null check >> Deal with v4l2 subdevs dependencies >> >> V3: Fix link error when compiled as a module >> >> V4: Remove duplicated v4l2_device_disconnect >> in the em28xx_v4l2_fini >> >> V5: Move all the v4l2 resources management >> to the v4l2_dev->release() callback. >> >> V6: Address some Hans comments regarding >> video_unregister_device and struct v4l2_device >> inside the struct v4l2_device. >> >> I'm sending this v6 that way but I'm totally open >> to the Hilt approach, if it is a more desirable >> way to fix this issue. >> --- >> drivers/media/usb/em28xx/em28xx-cards.c | 3 +- >> drivers/media/usb/em28xx/em28xx-video.c | 232 +++++++++++++++--------- >> drivers/media/usb/em28xx/em28xx.h | 1 - >> 3 files changed, 151 insertions(+), 85 deletions(-) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c >> index ba9292e2a587..6e67cf0a1e04 100644 >> --- a/drivers/media/usb/em28xx/em28xx-cards.c >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c >> @@ -4120,7 +4120,6 @@ static void em28xx_usb_disconnect(struct usb_interface *intf) >> struct em28xx *dev; >> >> dev = usb_get_intfdata(intf); >> - usb_set_intfdata(intf, NULL); >> >> if (!dev) >> return; >> @@ -4148,6 +4147,8 @@ static void em28xx_usb_disconnect(struct usb_interface *intf) >> dev->dev_next = NULL; >> } >> kref_put(&dev->ref, em28xx_free_device); >> + >> + usb_set_intfdata(intf, NULL); >> } >> >> static int em28xx_usb_suspend(struct usb_interface *intf, >> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c >> index 6b84c3413e83..abf9b325eae4 100644 >> --- a/drivers/media/usb/em28xx/em28xx-video.c >> +++ b/drivers/media/usb/em28xx/em28xx-video.c >> @@ -1897,7 +1897,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv, >> strscpy(chip->name, "ac97", sizeof(chip->name)); >> else >> strscpy(chip->name, >> - dev->v4l2->v4l2_dev.name, sizeof(chip->name)); >> + dev->v4l2->v4l2_dev->name, sizeof(chip->name)); >> return 0; >> } >> >> @@ -2113,21 +2113,6 @@ static int radio_s_tuner(struct file *file, void *priv, >> return 0; >> } >> >> -/* >> - * em28xx_free_v4l2() - Free struct em28xx_v4l2 >> - * >> - * @ref: struct kref for struct em28xx_v4l2 >> - * >> - * Called when all users of struct em28xx_v4l2 are gone >> - */ >> -static void em28xx_free_v4l2(struct kref *ref) >> -{ >> - struct em28xx_v4l2 *v4l2 = container_of(ref, struct em28xx_v4l2, ref); >> - >> - v4l2->dev->v4l2 = NULL; >> - kfree(v4l2); >> -} >> - >> /* >> * em28xx_v4l2_open() >> * inits the device and starts isoc transfer >> @@ -2153,12 +2138,21 @@ static int em28xx_v4l2_open(struct file *filp) >> return -EINVAL; >> } >> >> + /* To prevent the case when the v4l2_device_put() has already being called, >> + * the ref is now 0, we call a v4l2_device_get, and end up accessing freed >> + * resources. Or straigth accessing a freed v4l2. >> + */ >> + if (!v4l2 || !kref_get_unless_zero(&v4l2->v4l2_dev.ref)) >> + return -ENODEV; >> + > > This should not be needed: once the video devices are unregistered, it is no longer > possible to open the video device. So this can't happen. If a thread[1] with the `open` syscall pass verification point at v4l2_open() before the deregistration in the `em28xx_v4l2_fini`.
And If for some reason (e.g page fault) the thread[1] get stuck and can't get the `dev->lock` before the thread[2](which is in `em28xx_v4l2_dev_release`).*
The thread[1] will end up accessing freed resources.
Thread[1] | Open() | | Time Thread[1] Thread[2] | if(vdev==NULL... em28xx_v4l2_fini(em28xx_v4l2_fini) | ▽ Thread[1] Thread[2] em28xx_v4l2_open(...) b2_video_unregister_device(...)
Thread[1] Thread[2] PAGE-FAULT v4l2_device_put(&v4l2->v4l2_dev)
Thread[1] Thread[2] PAGE-FAULT if (refcount_dec_and_test(...))
Thread[1] Thread[2] PAGE-FAULT em28xx_v4l2_dev_release(...)
Thread[1] Thread[2] PAGE-FAULT mutex_lock(&dev->lock);
Thread[1] Thread[2] mutex_lock_interruptible() em28xx_v4l2_dev_unregistr(dev);
Thread[1] Thread[2] mutex_lock_interruptible() mutex_unlock(&dev->lock)0
Thread[1] Thread[2] access freed resources kref_put(&dev->ref, em28xx_free_device)
The case above is possible, isn't it?
> >> em28xx_videodbg("open dev=%s type=%s users=%d\n", >> video_device_node_name(vdev), v4l2_type_names[fh_type], >> v4l2->users); >> >> - if (mutex_lock_interruptible(&dev->lock)) >> + if (mutex_lock_interruptible(&dev->lock)) { >> + v4l2_device_put(&v4l2->v4l2_dev); > > So this is also not needed... > >> return -ERESTARTSYS; >> + } >> >> ret = v4l2_fh_open(filp); >> if (ret) { >> @@ -2166,6 +2160,7 @@ static int em28xx_v4l2_open(struct file *filp) >> "%s: v4l2_fh_open() returned error %d\n", >> __func__, ret); >> mutex_unlock(&dev->lock); >> + v4l2_device_put(&v4l2->v4l2_dev); > > ...and neither is this. > >> return ret; >> } >> >> @@ -2187,8 +2182,6 @@ static int em28xx_v4l2_open(struct file *filp) >> v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio); >> } >> >> - kref_get(&dev->ref); >> - kref_get(&v4l2->ref); >> v4l2->users++; >> >> mutex_unlock(&dev->lock); >> @@ -2222,36 +2215,30 @@ static int em28xx_v4l2_fini(struct em28xx *dev) >> >> mutex_lock(&dev->lock); >> >> - v4l2_device_disconnect(&v4l2->v4l2_dev); >> - >> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE); >> >> - em28xx_v4l2_media_release(dev); >> - >> if (video_is_registered(&v4l2->radio_dev)) { >> - dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n", >> + dev_info(&dev->intf->dev, >> + "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->radio_dev)); >> - video_unregister_device(&v4l2->radio_dev); >> + vb2_video_unregister_device(&v4l2->radio_dev); >> } >> if (video_is_registered(&v4l2->vbi_dev)) { >> - dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n", >> + dev_info(&dev->intf->dev, >> + "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->vbi_dev)); >> - video_unregister_device(&v4l2->vbi_dev); >> + vb2_video_unregister_device(&v4l2->vbi_dev); >> } >> if (video_is_registered(&v4l2->vdev)) { >> - dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n", >> + dev_info(&dev->intf->dev, >> + "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->vdev)); >> - video_unregister_device(&v4l2->vdev); >> + vb2_video_unregister_device(&v4l2->vdev); >> } >> >> - v4l2_ctrl_handler_free(&v4l2->ctrl_handler); >> - v4l2_device_unregister(&v4l2->v4l2_dev); >> - >> - kref_put(&v4l2->ref, em28xx_free_v4l2); >> - >> mutex_unlock(&dev->lock); >> >> - kref_put(&dev->ref, em28xx_free_device); >> + v4l2_device_put(&v4l2->v4l2_dev); >> >> return 0; >> } >> @@ -2323,9 +2310,8 @@ static int em28xx_v4l2_close(struct file *filp) >> >> exit: >> v4l2->users--; >> - kref_put(&v4l2->ref, em28xx_free_v4l2); >> mutex_unlock(&dev->lock); >> - kref_put(&dev->ref, em28xx_free_device); >> + v4l2_device_put(&v4l2->v4l2_dev); > > ...and neither is this v4l2_device_put. > >> >> return 0; >> } >> @@ -2517,6 +2503,28 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr) >> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f); >> } >> >> +static void em28xx_v4l2_dev_unregister(struct em28xx *dev) >> +{ >> + struct em28xx_v4l2 *v4l2 = dev->v4l2; >> + >> + v4l2_device_unregister(&v4l2->v4l2_dev); >> + em28xx_v4l2_media_release(dev); >> + v4l2_ctrl_handler_free(&v4l2->ctrl_handler); >> +} >> + >> +static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev) >> +{ >> + struct em28xx *dev = v4l2_dev->dev->driver_data; >> + >> + mutex_lock(&dev->lock); >> + em28xx_v4l2_dev_unregister(dev); >> + kfree(dev->v4l2); >> + dev->v4l2 = NULL; >> + mutex_unlock(&dev->lock); >> + >> + kref_put(&dev->ref, em28xx_free_device); >> +} >> + >> static int em28xx_v4l2_init(struct em28xx *dev) >> { >> u8 val; >> @@ -2524,6 +2532,7 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> unsigned int maxw; >> struct v4l2_ctrl_handler *hdl; >> struct em28xx_v4l2 *v4l2; >> + struct v4l2_subdev *sd; >> >> if (dev->is_audio_only) { >> /* Shouldn't initialize IR for this interface */ >> @@ -2541,12 +2550,13 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> >> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL); >> if (!v4l2) { >> - mutex_unlock(&dev->lock); >> - return -ENOMEM; >> + ret = -ENOMEM; >> + goto err; >> } >> - kref_init(&v4l2->ref); >> + >> v4l2->dev = dev; >> dev->v4l2 = v4l2; >> + v4l2->v4l2_dev.release = em28xx_v4l2_dev_release; >> >> #ifdef CONFIG_MEDIA_CONTROLLER >> v4l2->v4l2_dev.mdev = dev->media_dev; >> @@ -2555,7 +2565,7 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> if (ret < 0) { >> dev_err(&dev->intf->dev, >> "Call to v4l2_device_register() failed!\n"); >> - goto err; >> + goto free_v4l2; >> } >> >> hdl = &v4l2->ctrl_handler; >> @@ -2574,25 +2584,53 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> >> /* request some modules */ >> >> - if (dev->has_msp34xx) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "msp3400", 0, msp3400_addrs); >> + if (dev->has_msp34xx) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "msp3400", 0, msp3400_addrs); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering 'msp34xx' v4l2 subdevice!\n"); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> >> - if (dev->board.decoder == EM28XX_SAA711X) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "saa7115_auto", 0, saa711x_addrs); >> + if (dev->board.decoder == EM28XX_SAA711X) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "saa7115_auto", 0, saa711x_addrs); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering 'EM28XX_SAA711X' v4l2 subdevice!\n"); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> >> - if (dev->board.decoder == EM28XX_TVP5150) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "tvp5150", 0, tvp5150_addrs); >> + if (dev->board.decoder == EM28XX_TVP5150) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "tvp5150", 0, tvp5150_addrs); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering 'EM28XX_TVP5150' v4l2 subdevice!\n"); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> >> - if (dev->board.adecoder == EM28XX_TVAUDIO) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "tvaudio", dev->board.tvaudio_addr, NULL); >> + if (dev->board.adecoder == EM28XX_TVAUDIO) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "tvaudio", dev->board.tvaudio_addr, NULL); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering 'EM28XX_TVAUDIO' v4l2 subdevice!\n"); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> >> /* Initialize tuner and camera */ >> >> @@ -2600,33 +2638,63 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> unsigned short tuner_addr = dev->board.tuner_addr; >> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT); >> >> - if (dev->board.radio.type) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "tuner", dev->board.radio_addr, >> - NULL); >> - >> - if (has_demod) >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "tuner", 0, >> - v4l2_i2c_tuner_addrs(ADDRS_DEMOD)); >> + if (dev->board.radio.type) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "tuner", dev->board.radio_addr, >> + NULL); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering '%s' v4l2 subdevice!\n", >> + dev->board.name); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> + >> + if (has_demod) { >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "tuner", 0, >> + v4l2_i2c_tuner_addrs(ADDRS_DEMOD)); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering '%s' v4l2 subdevice!\n", >> + dev->i2c_adap[dev->def_i2c_bus].name); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + } >> + >> if (tuner_addr == 0) { >> enum v4l2_i2c_tuner_type type = >> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV; >> - struct v4l2_subdev *sd; >> >> sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> &dev->i2c_adap[dev->def_i2c_bus], >> "tuner", 0, >> v4l2_i2c_tuner_addrs(type)); >> - >> - if (sd) >> + if (sd) { >> tuner_addr = v4l2_i2c_subdev_addr(sd); >> + } else { >> + dev_err(&dev->intf->dev, >> + "Error while registering '%s' v4l2 subdevice!\n", >> + dev->i2c_adap[dev->def_i2c_bus].name); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> + >> } else { >> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> - &dev->i2c_adap[dev->def_i2c_bus], >> - "tuner", tuner_addr, NULL); >> + sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev, >> + &dev->i2c_adap[dev->def_i2c_bus], >> + "tuner", tuner_addr, NULL); >> + if (!sd) { >> + dev_err(&dev->intf->dev, >> + "Error while registering '%s' v4l2 subdevice!\n", >> + dev->i2c_adap[dev->def_i2c_bus].name); >> + ret = -EINVAL; >> + goto unregister_dev; >> + } >> } >> >> em28xx_tuner_setup(dev, tuner_addr); >> @@ -2755,7 +2823,6 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> if (ret) >> goto unregister_dev; >> >> - /* allocate and fill video video_device struct */ >> em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video"); >> mutex_init(&v4l2->vb_queue_lock); >> mutex_init(&v4l2->vb_vbi_queue_lock); >> @@ -2768,7 +2835,6 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> if (dev->tuner_type != TUNER_ABSENT) >> v4l2->vdev.device_caps |= V4L2_CAP_TUNER; >> >> - >> /* disable inapplicable ioctls */ >> if (dev->is_webcam) { >> v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD); >> @@ -2889,26 +2955,26 @@ static int em28xx_v4l2_init(struct em28xx *dev) >> dev_info(&dev->intf->dev, >> "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->radio_dev)); >> - video_unregister_device(&v4l2->radio_dev); >> + vb2_video_unregister_device(&v4l2->radio_dev); >> } >> if (video_is_registered(&v4l2->vbi_dev)) { >> dev_info(&dev->intf->dev, >> "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->vbi_dev)); >> - video_unregister_device(&v4l2->vbi_dev); >> + vb2_video_unregister_device(&v4l2->vbi_dev); >> } >> if (video_is_registered(&v4l2->vdev)) { >> dev_info(&dev->intf->dev, >> "V4L2 device %s deregistered\n", >> video_device_node_name(&v4l2->vdev)); >> - video_unregister_device(&v4l2->vdev); >> + vb2_video_unregister_device(&v4l2->vdev); >> } >> >> - v4l2_ctrl_handler_free(&v4l2->ctrl_handler); >> - v4l2_device_unregister(&v4l2->v4l2_dev); >> -err: >> + em28xx_v4l2_dev_unregister(dev); >> +free_v4l2: >> + kfree(v4l2); >> dev->v4l2 = NULL; >> - kref_put(&v4l2->ref, em28xx_free_v4l2); >> +err: >> mutex_unlock(&dev->lock); >> return ret; >> } >> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h >> index ab167cd1f400..666b7eff55f4 100644 >> --- a/drivers/media/usb/em28xx/em28xx.h >> +++ b/drivers/media/usb/em28xx/em28xx.h >> @@ -549,7 +549,6 @@ struct em28xx_eeprom { >> #define EM28XX_RESOURCE_VBI 0x02 >> >> struct em28xx_v4l2 { >> - struct kref ref; >> struct em28xx *dev; >> >> struct v4l2_device v4l2_dev; >> > > With the suggested changes it should be ready (pending testing on my side). > > Regards, > > Hans >
| |