Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
2018年10月8日(月) 20:21 Hans Verkuil :
>
> On 10/05/2018 04:59 PM, Akinobu Mita wrote:
> > 2018年10月5日(金) 18:36 Sakari Ailus :
> >>
> >> Hi Hans,
> >>
> >> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> >>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> The video_i2c_data is allocated by kzalloc and released by the video
> device's release callback. The release callback is called when
> video_unregister_device() is called, but it will still be accessed after
> calling video_unregister_device().
>
> Fix the use after free by allocating video_i2c_data by devm_kzalloc()
> with
> i2c_client->dev so that it will automatically be released when the i2c
> driver is removed.
> >>>
> >>> Hmm. The patch is right, but the explanation isn't. The core problem is
> >>> that vdev.release was set to video_i2c_release, but that should only be
> >>> used if struct video_device was kzalloc'ed. But in this case it is
> >>> embedded
> >>> in a larger struct, and then vdev.release should always be set to
> >>> video_device_release_empty.
> >>
> >> When the driver is unbound, what's acquired using the devm_() family of
> >> functions is released. At the same time, the user still holds a file
> >> handle, and can issue IOCTLs --- but the device's data structures no longer
> >> exist.
> >>
> >> That's not ok, and also the reason why we have the release callback.
> >>
> >> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> >> fine.
> >>
> >> Or am I missing something?
> >
> > How about moving the lines causing use-after-free to release callback
> > like below?
> >
> > static void video_i2c_release(struct video_device *vdev)
> > {
> > struct video_i2c_data *data = video_get_drvdata(vdev);
> >
> > v4l2_device_unregister(>v4l2_dev);
> > mutex_destroy(>lock);
> > mutex_destroy(>queue_lock);
> > kfree(data);
> > }
> >
>
> You can test this with v4l2-ctl:
>
> v4l2-ctl --sleep 10
>
> This sleeps 10s, then calls QUERYCAP and closes the file handle.
>
> In another shell you can unbind the driver while v4l2-ctl is sleeping.
>
> Hopefully this works without crashing anything :-)
I tried that and the command finished without crash.
$ v4l2-ctl --sleep 10 -d /dev/video2
Test VIDIOC_QUERYCAP:
VIDIOC_QUERYCAP returned -1 (No such device)
VIDIOC_QUERYCAP: No such device
This -ENODEV should be ok as V4L2_FL_REGISTERED flag has already been
cleared by video_unregister_device().
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
On 10/05/2018 04:59 PM, Akinobu Mita wrote:
> 2018年10月5日(金) 18:36 Sakari Ailus :
>>
>> Hi Hans,
>>
>> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
>>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
The video_i2c_data is allocated by kzalloc and released by the video
device's release callback. The release callback is called when
video_unregister_device() is called, but it will still be accessed after
calling video_unregister_device().
Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
i2c_client->dev so that it will automatically be released when the i2c
driver is removed.
>>>
>>> Hmm. The patch is right, but the explanation isn't. The core problem is
>>> that vdev.release was set to video_i2c_release, but that should only be
>>> used if struct video_device was kzalloc'ed. But in this case it is embedded
>>> in a larger struct, and then vdev.release should always be set to
>>> video_device_release_empty.
>>
>> When the driver is unbound, what's acquired using the devm_() family of
>> functions is released. At the same time, the user still holds a file
>> handle, and can issue IOCTLs --- but the device's data structures no longer
>> exist.
>>
>> That's not ok, and also the reason why we have the release callback.
>>
>> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
>> fine.
>>
>> Or am I missing something?
>
> How about moving the lines causing use-after-free to release callback
> like below?
>
> static void video_i2c_release(struct video_device *vdev)
> {
> struct video_i2c_data *data = video_get_drvdata(vdev);
>
> v4l2_device_unregister(>v4l2_dev);
> mutex_destroy(>lock);
> mutex_destroy(>queue_lock);
> kfree(data);
> }
>
You can test this with v4l2-ctl:
v4l2-ctl --sleep 10
This sleeps 10s, then calls QUERYCAP and closes the file handle.
In another shell you can unbind the driver while v4l2-ctl is sleeping.
Hopefully this works without crashing anything :-)
Regards,
Hans
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
Hi Mita-san,
On Fri, Oct 05, 2018 at 11:59:20PM +0900, Akinobu Mita wrote:
> 2018年10月5日(金) 18:36 Sakari Ailus :
> >
> > Hi Hans,
> >
> > On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > > The video_i2c_data is allocated by kzalloc and released by the video
> > > > device's release callback. The release callback is called when
> > > > video_unregister_device() is called, but it will still be accessed after
> > > > calling video_unregister_device().
> > > >
> > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc()
> > > > with
> > > > i2c_client->dev so that it will automatically be released when the i2c
> > > > driver is removed.
> > >
> > > Hmm. The patch is right, but the explanation isn't. The core problem is
> > > that vdev.release was set to video_i2c_release, but that should only be
> > > used if struct video_device was kzalloc'ed. But in this case it is
> > > embedded
> > > in a larger struct, and then vdev.release should always be set to
> > > video_device_release_empty.
> >
> > When the driver is unbound, what's acquired using the devm_() family of
> > functions is released. At the same time, the user still holds a file
> > handle, and can issue IOCTLs --- but the device's data structures no longer
> > exist.
> >
> > That's not ok, and also the reason why we have the release callback.
> >
> > While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> > fine.
> >
> > Or am I missing something?
>
> How about moving the lines causing use-after-free to release callback
> like below?
>
> static void video_i2c_release(struct video_device *vdev)
> {
> struct video_i2c_data *data = video_get_drvdata(vdev);
>
> v4l2_device_unregister(>v4l2_dev);
> mutex_destroy(>lock);
> mutex_destroy(>queue_lock);
> kfree(data);
> }
So the remove function would only contain video_unregister_device()? That's
the way it should be, as far as I can see.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
2018年10月5日(金) 18:36 Sakari Ailus :
>
> Hi Hans,
>
> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > The video_i2c_data is allocated by kzalloc and released by the video
> > > device's release callback. The release callback is called when
> > > video_unregister_device() is called, but it will still be accessed after
> > > calling video_unregister_device().
> > >
> > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > > i2c_client->dev so that it will automatically be released when the i2c
> > > driver is removed.
> >
> > Hmm. The patch is right, but the explanation isn't. The core problem is
> > that vdev.release was set to video_i2c_release, but that should only be
> > used if struct video_device was kzalloc'ed. But in this case it is embedded
> > in a larger struct, and then vdev.release should always be set to
> > video_device_release_empty.
>
> When the driver is unbound, what's acquired using the devm_() family of
> functions is released. At the same time, the user still holds a file
> handle, and can issue IOCTLs --- but the device's data structures no longer
> exist.
>
> That's not ok, and also the reason why we have the release callback.
>
> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> fine.
>
> Or am I missing something?
How about moving the lines causing use-after-free to release callback
like below?
static void video_i2c_release(struct video_device *vdev)
{
struct video_i2c_data *data = video_get_drvdata(vdev);
v4l2_device_unregister(>v4l2_dev);
mutex_destroy(>lock);
mutex_destroy(>queue_lock);
kfree(data);
}
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
Hi Hans, On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote: > On 09/23/2018 06:34 PM, Akinobu Mita wrote: > > The video_i2c_data is allocated by kzalloc and released by the video > > device's release callback. The release callback is called when > > video_unregister_device() is called, but it will still be accessed after > > calling video_unregister_device(). > > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with > > i2c_client->dev so that it will automatically be released when the i2c > > driver is removed. > > Hmm. The patch is right, but the explanation isn't. The core problem is > that vdev.release was set to video_i2c_release, but that should only be > used if struct video_device was kzalloc'ed. But in this case it is embedded > in a larger struct, and then vdev.release should always be set to > video_device_release_empty. When the driver is unbound, what's acquired using the devm_() family of functions is released. At the same time, the user still holds a file handle, and can issue IOCTLs --- but the device's data structures no longer exist. That's not ok, and also the reason why we have the release callback. While there are issues elsewhere, this bit of the V4L2 / MC frameworks is fine. Or am I missing something? -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
On 10/02/18 18:17, Akinobu Mita wrote: > 2018年10月1日(月) 18:40 Hans Verkuil : >> >> On 09/23/2018 06:34 PM, Akinobu Mita wrote: >>> The video_i2c_data is allocated by kzalloc and released by the video >>> device's release callback. The release callback is called when >>> video_unregister_device() is called, but it will still be accessed after >>> calling video_unregister_device(). >>> >>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with >>> i2c_client->dev so that it will automatically be released when the i2c >>> driver is removed. >> >> Hmm. The patch is right, but the explanation isn't. The core problem is >> that vdev.release was set to video_i2c_release, but that should only be >> used if struct video_device was kzalloc'ed. But in this case it is embedded >> in a larger struct, and then vdev.release should always be set to >> video_device_release_empty. >> >> That was the real reason for the invalid access. > > How about the commit log below? > > struct video_device for video-i2c is embedded in a struct video_i2c_data, > and then vdev.release should always be set to video_device_release_empty. > > However, the vdev.release is currently set to video_i2c_release which > frees the whole struct video_i2c_data. In video_i2c_remove(), some fields > (v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still > accessed after video_unregister_device(). > > This fixes the use after free by setting the vdev.release to > video_device_release_empty. Also, convert to use devm_kzalloc() for > allocating a struct video_i2c_data in order to simplify the code. > Looks good. Regards, Hans
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
2018年10月1日(月) 18:40 Hans Verkuil : > > On 09/23/2018 06:34 PM, Akinobu Mita wrote: > > The video_i2c_data is allocated by kzalloc and released by the video > > device's release callback. The release callback is called when > > video_unregister_device() is called, but it will still be accessed after > > calling video_unregister_device(). > > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with > > i2c_client->dev so that it will automatically be released when the i2c > > driver is removed. > > Hmm. The patch is right, but the explanation isn't. The core problem is > that vdev.release was set to video_i2c_release, but that should only be > used if struct video_device was kzalloc'ed. But in this case it is embedded > in a larger struct, and then vdev.release should always be set to > video_device_release_empty. > > That was the real reason for the invalid access. How about the commit log below? struct video_device for video-i2c is embedded in a struct video_i2c_data, and then vdev.release should always be set to video_device_release_empty. However, the vdev.release is currently set to video_i2c_release which frees the whole struct video_i2c_data. In video_i2c_remove(), some fields (v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still accessed after video_unregister_device(). This fixes the use after free by setting the vdev.release to video_device_release_empty. Also, convert to use devm_kzalloc() for allocating a struct video_i2c_data in order to simplify the code.
Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver
On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> The video_i2c_data is allocated by kzalloc and released by the video
> device's release callback. The release callback is called when
> video_unregister_device() is called, but it will still be accessed after
> calling video_unregister_device().
>
> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> i2c_client->dev so that it will automatically be released when the i2c
> driver is removed.
Hmm. The patch is right, but the explanation isn't. The core problem is
that vdev.release was set to video_i2c_release, but that should only be
used if struct video_device was kzalloc'ed. But in this case it is embedded
in a larger struct, and then vdev.release should always be set to
video_device_release_empty.
That was the real reason for the invalid access.
Regards,
Hans
>
> Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> Cc: Matt Ranostay
> Cc: Sakari Ailus
> Cc: Hans Verkuil
> Cc: Mauro Carvalho Chehab
> Acked-by: Matt Ranostay
> Signed-off-by: Akinobu Mita
> ---
> * v2
> - Update commit log to clarify the use after free
> - Add Acked-by tag
>
> 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(>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 = _i2c_chip[id->driver_data];
> else
> - goto error_free_device;
> + return -ENODEV;
>
> data->client = client;
> v4l2_dev = >v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>
> ret = v4l2_device_register(>dev, v4l2_dev);
> if (ret < 0)
> - goto error_free_device;
> + return ret;
>
> mutex_init(>lock);
> mutex_init(>queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
> data->vdev.fops = _i2c_fops;
> data->vdev.lock = >lock;
> data->vdev.ioctl_ops = _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(>lock);
> mutex_destroy(>queue_lock);
>
> -error_free_device:
> - kfree(data);
> -
> return ret;
> }
>
>
