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@vger.kernel.org Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com --- drivers/usb/typec/mux.c | 10 ++++++---- include/linux/usb/typec_mux.h | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index 2ce54f3fc79c..617687c4eb20 100644 --- a/drivers/usb/typec/mux.c +++ b/drivers/usb/typec/mux.c @@ -63,7 +63,8 @@ struct typec_switch *typec_switch_get(struct device *dev) sw = device_connection_find_match(dev, "orientation-switch", NULL, typec_switch_match); if (!IS_ERR_OR_NULL(sw)) { - WARN_ON(!try_module_get(sw->dev->driver->owner)); + sw->module = sw->dev->driver->owner; + WARN_ON(!try_module_get(sw->module)); get_device(sw->dev); } mutex_unlock(&switch_lock); @@ -81,7 +82,7 @@ EXPORT_SYMBOL_GPL(typec_switch_get); void typec_switch_put(struct typec_switch *sw) { if (!IS_ERR_OR_NULL(sw)) { - module_put(sw->dev->driver->owner); + module_put(sw->module); put_device(sw->dev); } } @@ -206,7 +207,8 @@ struct typec_mux *typec_mux_get(struct device *dev, mux = device_connection_find_match(dev, "mode-switch", (void *)desc, typec_mux_match); if (!IS_ERR_OR_NULL(mux)) { - WARN_ON(!try_module_get(mux->dev->driver->owner)); + mux->module = mux->dev->driver->owner; + WARN_ON(!try_module_get(mux->module)); get_device(mux->dev); } mutex_unlock(&mux_lock); @@ -224,7 +226,7 @@ EXPORT_SYMBOL_GPL(typec_mux_get); void typec_mux_put(struct typec_mux *mux) { if (!IS_ERR_OR_NULL(mux)) { - module_put(mux->dev->driver->owner); + module_put(mux->module); put_device(mux->dev); } } diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h index 43f40685e53c..b31498020bf3 100644 --- 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;
int (*set)(struct typec_mux *mux, int state);
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@vger.kernel.org Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com
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
On Thu, Mar 28, 2019 at 01:02:50PM +0100, Greg KH wrote:
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@vger.kernel.org Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com
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 :(
OK. I'll see if I can prepare a proper fix for the original.
thanks,
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Heikki Krogerus