Re: [PATCH 2/2] platform/chrome: cros_ec_chardev: Fix UAF of ec_dev

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jul 03, 2025 at 01:14:42PM +0000, Tzung-Bi Shih wrote:
> On Thu, Jul 03, 2025 at 01:52:02PM +0200, Greg KH wrote:
> > On Thu, Jul 03, 2025 at 11:35:09AM +0000, Tzung-Bi Shih wrote:
> > But yes, one that people have been talking about and discussing generic
> > ways of solving for years now, you have seen the plumbers talks about
> > it, right?
> 
> Will check them.
> 
> > > @@ -31,7 +34,14 @@
> > >  /* Arbitrary bounded size for the event queue */
> > >  #define CROS_MAX_EVENT_LEN	PAGE_SIZE
> > >  
> > > +/* This protects 'chardev_list' */
> > > +static DEFINE_MUTEX(chardev_lock);
> > > +static LIST_HEAD(chardev_list);
> > 
> > Having a static list of chardevices feels very odd and driver-specific,
> > right
> 
> The `chardev_list` is for recording all opened instances. Adding/removing
> entries in the .open()/.release() fops. The `chardev_lock` is for protecting
> from accessing the list simultaneously.

Ick, don't attempt to track objects across open/release, as that's not
the driver's job, that's the owner of the object that is doing the
open/release (i.e. the cdev) job.  You "know" that when open/release
happens, your object is "alive".  Any other time, you have no idea, so
don't attempt to try to track that please.

> They are statically allocated because they can't follow the lifecycle of
> neither the platform_device (e.g. can be gone after unbinding the driver)
> nor the chardev (e.g. can be gone after closing the file).

Yes, and your object should not have 2 reference counts, which is why
this is a common issue.  Your single object is trying to span both of
them, and the interactions is "messy".

You should be able to do this without the external list.  I know many
other drivers/subsystems have handled this properly, perhaps look at
them?

Or better yet, split the thing apart into your platform and misc device
portions?

Or even better, don't worry abouut it as NO ONE should be unbinding a
platform device from a driver under real operations.  If that does
happen, they could be, and should be, doing worse things to your system.
This is not a real-world situation that ever should be happening EXCEPT
for maybe when you are doing driver development work.  So it's a very
very very low priority issue.

thanks,

greg k-h