| Message ID | 20190328115208.42086-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: mux: Store module handle | expand |
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,
On Fri, Mar 29, 2019 at 02:17:01PM +0000, Sasha Levin wrote: > Hi, > > [This is an automated email] > > This commit has been processed because it contains a "Fixes:" tag, > fixing commit: . > > The bot has tested the following trees: v5.0.5, v4.19.32, v4.14.109, v4.9.166, v4.4.177, v3.18.137. > > v5.0.5: Build OK! > v4.19.32: Build OK! > v4.14.109: Failed to apply! Possible dependencies: > 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference counting") > 44262fad12a7 ("staging: typec: tcpm: Drop commented out code") > 4b4e02c83167 ("typec: tcpm: Move out of staging") > 5fd54ace4721 ("USB: add SPDX identifiers to all remaining files in drivers/usb/") > 70cd90be3300 ("staging: typec: pd: Document struct pd_message") > 98076fa64a05 ("staging: typec: tcpm: Document data structures") > bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers") > > v4.9.166: Failed to apply! Possible dependencies: > 02d5be466b68 ("staging: typec: tcpm: Prevent TCPM from looping in SRC_TRYWAIT") > 050161ea3268 ("staging: typec: tcpm: Fix Port Power Role field in PS_RDY messages") > 131c7d12ef21 ("staging: typec: tcpm: Follow Try.SRC exit requirements") > 193a68011fdc ("staging: typec: tcpm: Respond to Discover Identity commands") > 23b028c871e1 ("staging: bcm2835-audio: initial staging submission") > 32774ef3e4bb ("staging: vc04_services: use bcm2835 consequently") > 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference counting") > 4b4e02c83167 ("typec: tcpm: Move out of staging") > 5383fae76b82 ("doc-rst: fixed kernel-doc directives in usb/typec.rst") > 5fec4b54d0bf ("staging: typec: tcpm: Drop duplicate PD messages") > 70cd90be3300 ("staging: typec: pd: Document struct pd_message") > 74e656d6b055 ("staging: typec: Type-C Port Controller Interface driver (tcpci)") > 9b0ae69909c2 ("staging: typec: tcpm: set port type callback") > a0a3e04e6b2c ("staging: typec: tcpm: Check for Rp for tPDDebounce") > b17dd57118fe ("staging: typec: tcpm: Improve role swap with non PD capable partners") > b965b6317ff1 ("staging: typec: tcpm: typec: tcpm: Wait for CC debounce before PD excg") > bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers") > c034a43e72dd ("staging: typec: Fairchild FUSB302 Type-c chip driver") > c749d4d0e4b8 ("staging: typec: tcpm: Set default state after error recovery based on port type") > c79d92bda80c ("staging: typec: tcpm: Check cc status before entering SRC_TRY_DEBOUCE") > f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)") > fab9288428ec ("usb: USB Type-C connector class") > fce042f02ef0 ("staging: typec: tcpm: Report right typec_pwr_opmode") > > v4.4.177: Failed to apply! Possible dependencies: > 13a9930d15b4 ("staging: ks7010: add driver from Nanonote extra-repository") > 199d68d4a8cb ("greybus: start moving the function types into the greybus core") > 23b028c871e1 ("staging: bcm2835-audio: initial staging submission") > 27fb83109a39 ("greybus: register the bus with the driver core and add framework for debugfs files.") > 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference counting") > 4b4e02c83167 ("typec: tcpm: Move out of staging") > 53419e07cc94 ("greybus: i2c-gb: actually add the i2c adapter properly...") > 5383fae76b82 ("doc-rst: fixed kernel-doc directives in usb/typec.rst") > 71bad7f08641 ("staging: add bcm2708 vchiq driver") > 79c822be7b85 ("greybus: uart framework added, doesn't build") > 83ddaaab01c2 ("greybus: Greybus SD/MMC host driver") > 85f37d17b3f1 ("isdn: act200: fix MODULE_PARM_DESC() typo") > a18e15175708 ("greybus: more uart work") > a921e9bd4e22 ("isdn: i4l: move active-isdn drivers to staging") > bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers") > c16854c3bf56 ("greybus: gpio driver") > c8a797a98cb6 ("greybus: Import most recent greybus code to new repo.") > d4f56b47a8fa ("staging: greybus: Add drivers/staging/greybus to the build") > d5d1903dcd15 ("greybus: add framework for 'struct gbuf'") > db6e1fd264ac ("greybus: hook up sdio, gpio, and tty into the greybus core.") > de536e309476 ("greybus: ap message loop added.") > e68453ed28c5 ("greybus: uart-gb: now builds, more framework added") > e9023d227ab8 ("greybus: gpio-gb.c: it now builds properly") > f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)") > fab9288428ec ("usb: USB Type-C connector class") > ff45c265f849 ("greybus: uart-gb: more work on tty functions") > > v3.18.137: Failed to apply! Possible dependencies: > 01ed1e1504ac ("isdn: icn: remove a #warning") > 10640d34552c ("isdn: icn: use strlcpy() when parsing setup options") > 13a9930d15b4 ("staging: ks7010: add driver from Nanonote extra-repository") > 23b028c871e1 ("staging: bcm2835-audio: initial staging submission") > 2584cf83578c ("arch, drivers: don't include <asm/io.h> directly, use <linux/io.h> instead") > 2cbf7fe2d5d3 ("i2o: move to staging") > 3e3b81965cbf ("usb: typec: mux: Take care of driver module reference counting") > 4b4e02c83167 ("typec: tcpm: Move out of staging") > 5383fae76b82 ("doc-rst: fixed kernel-doc directives in usb/typec.rst") > 57562a72414c ("Staging: most: add MOST driver's core module") > 71bad7f08641 ("staging: add bcm2708 vchiq driver") > 85f37d17b3f1 ("isdn: act200: fix MODULE_PARM_DESC() typo") > a921e9bd4e22 ("isdn: i4l: move active-isdn drivers to staging") > bbf9d17d9875 ("staging: fsl-mc: Freescale Management Complex (fsl-mc) bus driver") > bdecb33af34f ("usb: typec: API for controlling USB Type-C Multiplexers") > c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver") > d4f56b47a8fa ("staging: greybus: Add drivers/staging/greybus to the build") > f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)") > fab9288428ec ("usb: USB Type-C connector class") > > > How should we proceed with this patch? Greg already rejected this patch. thanks,
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);
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(-)