HID: rmi: Check that the RMI_STARTED bit is set before unregistering the RMI transport device
diff mbox series

Message ID 20191023012344.20998-1-aduggan@synaptics.com
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
Related show

Commit Message

Andrew Duggan Oct. 23, 2019, 1:24 a.m. UTC
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(-)

Comments

Jiri Kosina Nov. 15, 2019, 3:26 p.m. UTC | #1
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,
Andrew Duggan Nov. 15, 2019, 6:18 p.m. UTC | #2
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
>
Jiri Kosina Nov. 18, 2019, 9:24 a.m. UTC | #3
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,

Patch
diff mbox series

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);