Re: cdev/devm_* issues (was Re: [TECH TOPIC] Rust for Linux)

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sat, Jul 10, 2021 at 10:09:10AM +0300, Dan Carpenter wrote:
> The other thing which is quite tricky to get right is kobj releases.  I
> have a bunch of these static checker warning but they're annoying
> because they're not something which really affects real life so I can't
> be bothered to fix or report them.

I'm not a security researcher, but I often wonder if there are ways
root could exploit these kinds of error unwind bugs during module
loading to exploit the kernel? That is a relevant concern in the
higher integrity modes.

>   1045          if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
>   1046                  pr_warn("%s: requested %s%d, got %s\n", __func__,
>   1047                          name_base, nr, video_device_node_name(vdev));
>   1048  
>   1049          /* Increase v4l2_device refcount */
>   1050          v4l2_device_get(vdev->v4l2_dev);
>   1051  
>   1052          /* Part 5: Register the entity. */
>   1053          ret = video_register_media_controller(vdev);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> But then there is no way to clean up from this is
> video_register_media_controller() fails.  If we call put_device() it
> would lead to use after frees in the callers.  We just have to ignore
> the error.

Eg if we ignore these errors and keep going is the kernel object in a
bad state that userspace can exploit?

The solution here is the same as the other cases, use device_add() not
device_register(), put a device_initialize() immediately after the
allocation of vdev and audit/fix the release so it can work on
partially initialized objects.

The idea of a video_register_device() is just not right in a subsystem
so complicated the 'init & register' pattern is a shortcut to save a
few lines in simple drivers. It should never be baked into a subsystem
like this and it certainly shouldn't work the opposide of how
device_register() works: "if video_register_device fails, the
release() callback of &struct video_device structure is *not* called"

Which is why the release function assignment is out of order, and it
is forced to print a warning in a certain error flow :\

> It's a minor thing, but it's so frustrating.

Yes :(

And as others mentioned devm has its own set of bug classes - I've
seen issues with the sequencing of the devm unloads.. Especially when
work queues get involved.

Jason