| Message ID | 20191023012344.20998-1-aduggan@synaptics.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 8725aa4fa7ded30211ebd28bb1c9bae806eb3841 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: rmi: Check that the RMI_STARTED bit is set before unregistering the RMI transport device | expand |
On Wed, 23 Oct 2019, Andrew Duggan wrote: > In the event that the RMI device is unreachable, the calls to > rmi_set_mode() or rmi_set_page() will fail before registering the RMI > transport device. When the device is removed, rmi_remove() will call > rmi_unregister_transport_device() which will attempt to access the > rmi_dev pointer which was not set. This patch adds a check of the > RMI_STARTED bit before calling rmi_unregister_transport_device(). > The RMI_STARTED bit is only set after rmi_register_transport_device() > completes successfully. A subsequent patch in the RMI core will add > checks to validate the pointers before accessing them. Andrew, my mailbox doesn't seem to have that 'subsequent patch' ... was it ever sent and I missed it? If so, could you please resend it? Thanks,
Hi Jiri, On 11/15/19 7:26 AM, Jiri Kosina wrote: > On Wed, 23 Oct 2019, Andrew Duggan wrote: > >> In the event that the RMI device is unreachable, the calls to >> rmi_set_mode() or rmi_set_page() will fail before registering the RMI >> transport device. When the device is removed, rmi_remove() will call >> rmi_unregister_transport_device() which will attempt to access the >> rmi_dev pointer which was not set. This patch adds a check of the >> RMI_STARTED bit before calling rmi_unregister_transport_device(). >> The RMI_STARTED bit is only set after rmi_register_transport_device() >> completes successfully. A subsequent patch in the RMI core will add >> checks to validate the pointers before accessing them. > Andrew, > > my mailbox doesn't seem to have that 'subsequent patch' ... was it ever > sent and I missed it? If so, could you please resend it? The subsequent patch which I was referring to is: https://lore.kernel.org/linux-input/20191023012344.20998-2-aduggan@synaptics.com/ Since this second patch was for the input subsystem I decided to just make them separate patches instead of creating a series. However, based on Dmitry's feedback it was determined that the second patch wasn't a good idea and it won't be applied. This first patch is enough to fix the issue by preventing the call to rmi_unregister_transport_device() if the subsequent call to register failed. The only change I would make to this patch would be to remove the last sentence of the comment. If you choose to apply that patch then would this be a change you would make? Or would you prefer I submit a v2 with this update? Thanks, Andrew > Thanks, > > -- > Jiri Kosina > SUSE Labs >
On Fri, 15 Nov 2019, Andrew Duggan wrote: > Since this second patch was for the input subsystem I decided to just > make them separate patches instead of creating a series. However, based > on Dmitry's feedback it was determined that the second patch wasn't a > good idea and it won't be applied. This first patch is enough to fix the > issue by preventing the call to rmi_unregister_transport_device() if the > subsequent call to register failed. The only change I would make to this > patch would be to remove the last sentence of the comment. If you choose > to apply that patch then would this be a change you would make? Or would > you prefer I submit a v2 with this update? I've modified the changelog and applied. Thanks,
diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c index 7c6abd7e0979..9ce22acdfaca 100644 --- a/drivers/hid/hid-rmi.c +++ b/drivers/hid/hid-rmi.c @@ -744,7 +744,8 @@ static void rmi_remove(struct hid_device *hdev) { struct rmi_data *hdata = hid_get_drvdata(hdev); - if (hdata->device_flags & RMI_DEVICE) { + if ((hdata->device_flags & RMI_DEVICE) + && test_bit(RMI_STARTED, &hdata->flags)) { clear_bit(RMI_STARTED, &hdata->flags); cancel_work_sync(&hdata->reset_work); rmi_unregister_transport_device(&hdata->xport);
In the event that the RMI device is unreachable, the calls to rmi_set_mode() or rmi_set_page() will fail before registering the RMI transport device. When the device is removed, rmi_remove() will call rmi_unregister_transport_device() which will attempt to access the rmi_dev pointer which was not set. This patch adds a check of the RMI_STARTED bit before calling rmi_unregister_transport_device(). The RMI_STARTED bit is only set after rmi_register_transport_device() completes successfully. A subsequent patch in the RMI core will add checks to validate the pointers before accessing them. The kernel oops was reported in this message: https://www.spinics.net/lists/linux-input/msg58433.html Signed-off-by: Andrew Duggan <aduggan@synaptics.com> Reported-by: Federico Cerutti <federico@ceres-c.it> --- drivers/hid/hid-rmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)