Re: [PATCH v3 1/2] media: v4l2-subdev: Fix NULL pointer dereference in subdev_open()

[Nicola Fiorillo <nicfio@gmail.com>]

Hi Laurent,

Thank you for looking at it.

On 6.12.86: agreed, that line does not belong in the commit message.
The runs that matter are the ones on media next described in the cover
letter, and those are what the commit message should have referred to.

On the fix: agreed as well, it does not fix the problem. subdev_open()
now goes through vdev->v4l2_dev->mdev, which is only valid as long as
the driver keeps the v4l2_device and the media_device alive. vimc does,
through its v4l2_device release callback, but ipu6-isys embeds both in
struct ipu6_isys, allocated with devm_kzalloc(), so they are freed when
the driver is unbound.

I checked this on the IPU6 tablet (media next 9cfc1aca0781 plus the
sensor drivers mentioned in the cover letter, without this series):
with the media device, a video node and a CSI-2 sub-device node of
isys held open, I unbound isys, then issued one ioctl on each node and
closed them. KASAN reports use-after-free in v4l2_ioctl() on the video
node, then on close in v4l2_release(), v4l2_prio_close() and
v4l2_device_release() on the struct ipu6_isys allocated in
isys_probe() and freed by devres at unbind, and more in subdev_close()
and __vb2_queue_free().

Patch 2/2 rests on the same vdev->v4l2_dev assumption, so I am
withdrawing the whole series rather than keeping half of it. The
underlying issue is the lifetime of the objects in the driver, and
that needs a proper fix along the lines of what Hans did for em28xx,
not another check in the file operations.

Thanks,
Nicola