diff mbox series

[1/5] media: video-i2c: avoid accessing released memory area when removing driver

Message ID 1537200191-17956-2-git-send-email-akinobu.mita@gmail.com (mailing list archive)
State New, archived
Headers show
Series media: video-i2c: support changing frame interval and runtime PM | expand

Commit Message

Akinobu Mita Sept. 17, 2018, 4:03 p.m. UTC 
The struct video_i2c_data is released when video_unregister_device() is
called, but it will still be accessed after calling
video_unregister_device().

Use devm_kzalloc() and let the memory be automatically released on driver
detach.

Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

Comments

Matt Ranostay Sept. 17, 2018, 5:32 p.m. UTC | #1 
On Mon, Sep 17, 2018 at 9:03 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> The struct video_i2c_data is released when video_unregister_device() is
> called, but it will still be accessed after calling
> video_unregister_device().
>
> Use devm_kzalloc() and let the memory be automatically released on driver
> detach.
>
> Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> ---
>  drivers/media/i2c/video-i2c.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..b7a2af9 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>         .vidioc_streamoff               = vb2_ioctl_streamoff,
>  };
>
> -static void video_i2c_release(struct video_device *vdev)
> -{
> -       kfree(video_get_drvdata(vdev));
> -}
> -
>  static int video_i2c_probe(struct i2c_client *client,
>                              const struct i2c_device_id *id)
>  {
>         struct video_i2c_data *data;
>         struct v4l2_device *v4l2_dev;
>         struct vb2_queue *queue;
> -       int ret = -ENODEV;
> +       int ret;
>
> -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>         if (!data)
>                 return -ENOMEM;
>
> @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
>         else if (id)
>                 data->chip = &video_i2c_chip[id->driver_data];
>         else
> -               goto error_free_device;
> +               return -ENODEV;
>
>         data->client = client;
>         v4l2_dev = &data->v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>
>         ret = v4l2_device_register(&client->dev, v4l2_dev);
>         if (ret < 0)
> -               goto error_free_device;
> +               return ret;
>
>         mutex_init(&data->lock);
>         mutex_init(&data->queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
>         data->vdev.fops = &video_i2c_fops;
>         data->vdev.lock = &data->lock;
>         data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> -       data->vdev.release = video_i2c_release;
> +       data->vdev.release = video_device_release_empty;
>         data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
>                                  V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
>
> @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
>         mutex_destroy(&data->lock);
>         mutex_destroy(&data->queue_lock);
>
> -error_free_device:
> -       kfree(data);
> -
>         return ret;
>  }
>
> --
> 2.7.4
>
Sakari Ailus Sept. 19, 2018, 10:35 a.m. UTC | #2 
Hi Mita-san,

On Tue, Sep 18, 2018 at 01:03:07AM +0900, Akinobu Mita wrote:
> The struct video_i2c_data is released when video_unregister_device() is
> called, but it will still be accessed after calling
> video_unregister_device().
> 
> Use devm_kzalloc() and let the memory be automatically released on driver
> detach.
> 
> Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..b7a2af9 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>  	.vidioc_streamoff		= vb2_ioctl_streamoff,
>  };
>  
> -static void video_i2c_release(struct video_device *vdev)
> -{
> -	kfree(video_get_drvdata(vdev));

This is actually correct: it ensures that that the device data stays in
place as long as the device is being accessed. Allocating device data with
devm_kzalloc() no longer guarantees that, and is not the right thing to do
for that reason.

> -}
> -
>  static int video_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
>  	struct video_i2c_data *data;
>  	struct v4l2_device *v4l2_dev;
>  	struct vb2_queue *queue;
> -	int ret = -ENODEV;
> +	int ret;
>  
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	else if (id)
>  		data->chip = &video_i2c_chip[id->driver_data];
>  	else
> -		goto error_free_device;
> +		return -ENODEV;
>  
>  	data->client = client;
>  	v4l2_dev = &data->v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  
>  	ret = v4l2_device_register(&client->dev, v4l2_dev);
>  	if (ret < 0)
> -		goto error_free_device;
> +		return ret;
>  
>  	mutex_init(&data->lock);
>  	mutex_init(&data->queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	data->vdev.fops = &video_i2c_fops;
>  	data->vdev.lock = &data->lock;
>  	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> -	data->vdev.release = video_i2c_release;
> +	data->vdev.release = video_device_release_empty;
>  	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
>  				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
>  
> @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
>  	mutex_destroy(&data->lock);
>  	mutex_destroy(&data->queue_lock);
>  
> -error_free_device:
> -	kfree(data);
> -
>  	return ret;
>  }
>
Akinobu Mita Sept. 19, 2018, 3 p.m. UTC | #3 
2018年9月19日(水) 19:35 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Tue, Sep 18, 2018 at 01:03:07AM +0900, Akinobu Mita wrote:
> > The struct video_i2c_data is released when video_unregister_device() is
> > called, but it will still be accessed after calling
> > video_unregister_device().
> >
> > Use devm_kzalloc() and let the memory be automatically released on driver
> > detach.
> >
> > Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Hans Verkuil <hansverk@cisco.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 18 +++++-------------
> >  1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 06d29d8..b7a2af9 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
> >       .vidioc_streamoff               = vb2_ioctl_streamoff,
> >  };
> >
> > -static void video_i2c_release(struct video_device *vdev)
> > -{
> > -     kfree(video_get_drvdata(vdev));
>
> This is actually correct: it ensures that that the device data stays in
> place as long as the device is being accessed. Allocating device data with
> devm_kzalloc() no longer guarantees that, and is not the right thing to do
> for that reason.

I have actually inserted printk() each line in video_i2_remove().  When
rmmod this driver, video_i2c_release() (and also kfree) is called while
executing video_unregister_device().  Because video_unregister_device()
releases the last reference to data->vdev.dev, then v4l2_device_release()
callback executes data->vdev.release.

So use after freeing video_i2c_data actually happened.

In this patch, devm_kzalloc() is called with client->dev (not with vdev->dev).
So the allocated memory is released when the last user of client->dev
is gone (maybe just after video_i2_remove() is finished).

> > -}
> > -
> >  static int video_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >  {
> >       struct video_i2c_data *data;
> >       struct v4l2_device *v4l2_dev;
> >       struct vb2_queue *queue;
> > -     int ret = -ENODEV;
> > +     int ret;
> >
> > -     data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +     data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> >       if (!data)
> >               return -ENOMEM;
> >
> > @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >       else if (id)
> >               data->chip = &video_i2c_chip[id->driver_data];
> >       else
> > -             goto error_free_device;
> > +             return -ENODEV;
> >
> >       data->client = client;
> >       v4l2_dev = &data->v4l2_dev;
> > @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >
> >       ret = v4l2_device_register(&client->dev, v4l2_dev);
> >       if (ret < 0)
> > -             goto error_free_device;
> > +             return ret;
> >
> >       mutex_init(&data->lock);
> >       mutex_init(&data->queue_lock);
> > @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >       data->vdev.fops = &video_i2c_fops;
> >       data->vdev.lock = &data->lock;
> >       data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> > -     data->vdev.release = video_i2c_release;
> > +     data->vdev.release = video_device_release_empty;
> >       data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
> >                                V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> >
> > @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
> >       mutex_destroy(&data->lock);
> >       mutex_destroy(&data->queue_lock);
> >
> > -error_free_device:
> > -     kfree(data);
> > -
> >       return ret;
> >  }
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
diff mbox series

Patch

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 06d29d8..b7a2af9 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -508,20 +508,15 @@  static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_streamoff		= vb2_ioctl_streamoff,
 };
 
-static void video_i2c_release(struct video_device *vdev)
-{
-	kfree(video_get_drvdata(vdev));
-}
-
 static int video_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
 	struct video_i2c_data *data;
 	struct v4l2_device *v4l2_dev;
 	struct vb2_queue *queue;
-	int ret = -ENODEV;
+	int ret;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -530,7 +525,7 @@  static int video_i2c_probe(struct i2c_client *client,
 	else if (id)
 		data->chip = &video_i2c_chip[id->driver_data];
 	else
-		goto error_free_device;
+		return -ENODEV;
 
 	data->client = client;
 	v4l2_dev = &data->v4l2_dev;
@@ -538,7 +533,7 @@  static int video_i2c_probe(struct i2c_client *client,
 
 	ret = v4l2_device_register(&client->dev, v4l2_dev);
 	if (ret < 0)
-		goto error_free_device;
+		return ret;
 
 	mutex_init(&data->lock);
 	mutex_init(&data->queue_lock);
@@ -568,7 +563,7 @@  static int video_i2c_probe(struct i2c_client *client,
 	data->vdev.fops = &video_i2c_fops;
 	data->vdev.lock = &data->lock;
 	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
-	data->vdev.release = video_i2c_release;
+	data->vdev.release = video_device_release_empty;
 	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
 				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
 
@@ -597,9 +592,6 @@  static int video_i2c_probe(struct i2c_client *client,
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
 
-error_free_device:
-	kfree(data);
-
 	return ret;
 }