On Thu, Mar 28, 2019 at 02:52:08PM +0300, Heikki Krogerus wrote:
> It is possible that the driver of the mux device has been
> unbind by the time typec_mux_put() or typec_switch_put() is
> called.
>
> To prevent the NULL Pointer Dereference from happening in
> this case when decrementing the reference count of the
> module by using dev->driver->owner, storing the module
> handle to the mux and switch data structures, and using the
> stored value instead.
>
> Fixes: ("3e3b81965cbf usb: typec: mux: Take care of driver module reference counting")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/typec/mux.c | 10 ++++++----
> include/linux/usb/typec_mux.h | 2 ++
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -20,6 +20,7 @@ struct device;
> */
> struct typec_switch {
> struct device *dev;
> + struct module *module;
> struct list_head entry;
>
> int (*set)(struct typec_switch *sw, enum typec_orientation orientation);
> @@ -37,6 +38,7 @@ struct typec_switch {
> */
> struct typec_mux {
> struct device *dev;
> + struct module *module;
> struct list_head entry;
You have just created two different reference counts for a single object
here :(
This is data, not code. Data needs to be protected with the reference
count to keep it from being freed while in use. Code also needs to be
protected, BUT, do not mix the two.
driver owners should always be properly reference counted if userspace
opens the device, not if another internal kernel code opens the device.
Or better yet, just properly unwind things when the code is removed, no
need to reference count anything on the module level (like networking
devices do it).
So I really do not think this patch is correct, and odds are, the
original one that this patch says it fixes is probably also not correct
:(
thanks,
greg k-h
