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

Jason Gunthorpe
2021-07-12T13:42:36Z

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