On Fri, Oct 04, 2024 at 11:17:01AM GMT, Thadeu Lima de Souza Cascardo wrote: > On Fri, Oct 04, 2024 at 05:08:28PM +0300, Heikki Krogerus wrote: > > On Fri, Oct 04, 2024 at 09:37:38AM -0300, Thadeu Lima de Souza Cascardo wrote: > > > The altmode device release refers to its parent device, but without keeping > > > a reference to it. > > > > > > When registering the altmode, get a reference to the parent and put it in > > > the release function. > > > > > > Before this fix, when using CONFIG_DEBUG_KOBJECT_RELEASE, we see issues > > > like this: > > > > Let me study what's going on in the drivers code. The children should > > _not_ be cleaned first before the parent. I'll have to come back to > > this on Monday. > > > > This really should not be necessary. > > > > Well, they are likely not. But driver core API states that either way, you > should keep such references. And one way to test it is using > CONFIG_DEBUG_KOBJECT_RELEASE. That delays the actual release/cleanup of the > struct device, so: > > > > > [ 43.572860] kobject: 'port0.0' (ffff8880057ba008): kobject_release, parent 0000000000000000 (delayed 3000) > > > [ 43.573532] kobject: 'port0.1' (ffff8880057bd008): kobject_release, parent 0000000000000000 (delayed 1000) > > > [ 43.574407] kobject: 'port0' (ffff8880057b9008): kobject_release, parent 0000000000000000 (delayed 3000) > > > [ 43.575059] kobject: 'port1.0' (ffff8880057ca008): kobject_release, parent 0000000000000000 (delayed 4000) > > > [ 43.575908] kobject: 'port1.1' (ffff8880057c9008): kobject_release, parent 0000000000000000 (delayed 4000) > > 1) children (port1.0 and port1.1) last reference are put, but their actual > release is delayed 4s. > > > > [ 43.576908] kobject: 'typec' (ffff8880062dbc00): kobject_release, parent 0000000000000000 (delayed 4000) > > > [ 43.577769] kobject: 'port1' (ffff8880057bf008): kobject_release, parent 0000000000000000 (delayed 3000) > > 2) parent (port1) is put, but release is delayed 3s. > > Just in the order you would expect, but because of the delays: > > 3) 3s later, port1 release is called and it is freed. > 4) 4s later, port1.0 release is called and it refers to the freed parent, > port1. > > Having the references, what happens now is: > > 1) port1 is put, but this is not its last reference. > 2) port1.0 and port1.1 are put, cleanup delayed 4s. > 3) 4s later, port1.0 and port1.1 releases are called, but now they put the > last reference to port1. > 4) port1 last reference has now been called, cleanup delayed 3s. > 5) 3s later, port1 release is called, then freed. > > No UAF in such case. Usually we don't use this config option, maybe I should also start using it for some of my tests. Nevertheless the description is pretty clear (although it might be better to add it to the commit message). Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> -- With best wishes Dmitry
