Hello, On Tue, Dec 21, 2021 at 01:04:50PM +0100, Lars-Peter Clausen wrote: > On 12/21/21 12:35 PM, Uwe Kleine-König wrote: > > On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote: > > > On 12/21/21 11:45 AM, Uwe Kleine-König wrote: > > > > similar to patch > > > > https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@xxxxxxxxx > > > > the usage of struct counter_device::priv can be replaced by > > > > container_of which improves type safety and code size. > > > > > > > > This series depends on above patch, converts the remaining drivers and > > > > finally drops struct counter_device::priv. > > > Not sure if this is such a good idea. struct counter_device should not be > > > embedded in the drivers state struct in the first place. > > Just to mention it: My patch series didn't change this, this was already > > broken before. > I know, but this series has to be reverted when the framework is fixed. > > > > > struct counter_device contains a struct device, which is a reference counted > > > object. But by embedding it in the driver state struct the life time of both > > > the struct counter_device and and struct device are bound to the life time > > > of the driver state struct. > > > > > > Which means the struct device memory can get freed before the last reference > > > is dropped, which leads to a use-after-free and undefined behavior. > > Well, the driver struct is allocated using devm_kzalloc for all drivers. > > devm_kzalloc() doesn't make a difference. The managed memory is freed when > the parent device is unbound/removed. There may very well be reference to > the counter_device at this point. > > > So I think it's not *very* urgent to fix. Still you're right, this > > should be addressed. > > Yes and no, this can trivially be used for privilege escalation, but then > again on systems with a counter_device probably everything runs as root > anyway. I could provoke an oops with the following shell command: { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind I have a protype here to split counter_register() into counter_alloc() + counter_add(), but I didn't convert a driver to it yet. If you want to take a look, it's currently available from https://git.pengutronix.de/git/ukl/linux counter-dev-livetime (or if you prefer a webif: https://git.pengutronix.de/cgit/ukl/linux/log/?h=counter-dev-livetime ). I planned to just invest a two or so hours to fix this. But the plan failed (one reason is that v5.16-rc6 failed to boot on the stm32mp1 I work on and I bisected that first.) Maybe I find some time between the years to get this forward a bit. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
