On Mon, 18 Nov 2019 at 11:15, Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> wrote: > > > > On 11/15/19 11:12 PM, Ezequiel Garcia wrote: > > Hi Dafna, > > > > Thanks for the patch! > > > > On Fri, 15 Nov 2019 at 13:13, Dafna Hirschfeld > > <dafna.hirschfeld@xxxxxxxxxxxxx> wrote: > >> > >> In case v4l2_device_register_subdev_nodes fails, sd->devnode is > >> unregistered and then released. Therefore the field is set to > >> NULL so that the subdev won't hold a pointer to a released object. > >> This fixes a reference after free bug in function > >> v4l2_device_unregister_subdev > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> > >> --- > >> drivers/media/v4l2-core/v4l2-device.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c > >> index 63d6b147b21e..468705c85e97 100644 > >> --- a/drivers/media/v4l2-core/v4l2-device.c > >> +++ b/drivers/media/v4l2-core/v4l2-device.c > >> @@ -250,6 +250,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) > >> if (!sd->devnode) > >> break; > >> video_unregister_device(sd->devnode); > >> + sd->devnode = NULL; > >> } > >> > > > > You also need to clear devnode in v4l2_device_unregister_subdev, as we > > discussed in IRC. > > It would mean fixing a different issue that is triggered upon driver > > removal/unbinding, > I think that the right place to set devnode to null would be > in the beginning of the function v4l2_device_release_subdev_node > where it is freed. This ensures that the devnode is set to NULL always > and only when it is freed. > What do you think? > Makes sense. Or also, you can add this in v4l2_subdev_release (called by v4l2_device_release_subdev_node) since it's modifying the subdev, might as well do it there to keep things even cleaner. Thanks, Ezequiel
