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 <[email protected]>
Reported-by: Federico Cerutti <[email protected]>
---
drivers/hid/hid-rmi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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);
--
2.20.1
A bug in hid-rmi was causing rmi_unregister_transport_device() to be
called even if the call to rmi_register_transport_device() failed to
allocate the rmi device. A patch has been submitted to fix the issue in
hid-rmi. This patch will ensure that should a simialr situation
occur then the rmi driver will not dereference a NULL pointer.
Signed-off-by: Andrew Duggan <[email protected]>
---
drivers/input/rmi4/rmi_bus.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index af706a583656..6c3abae1e159 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -118,8 +118,10 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
{
struct rmi_device *rmi_dev = xport->rmi_dev;
- device_del(&rmi_dev->dev);
- put_device(&rmi_dev->dev);
+ if (rmi_dev) {
+ device_del(&rmi_dev->dev);
+ put_device(&rmi_dev->dev);
+ }
}
EXPORT_SYMBOL(rmi_unregister_transport_device);
--
2.20.1
Hi Andrew,
On Wed, Oct 23, 2019 at 01:24:05AM +0000, Andrew Duggan wrote:
> A bug in hid-rmi was causing rmi_unregister_transport_device() to be
> called even if the call to rmi_register_transport_device() failed to
> allocate the rmi device. A patch has been submitted to fix the issue in
> hid-rmi. This patch will ensure that should a simialr situation
> occur then the rmi driver will not dereference a NULL pointer.
This looks like "garbage in, garbage out" problem where we should not be
calling unregister in the first place. I'd rather not apply this.
Thanks.
--
Dmitry
Hi Dmitry,
On 10/28/19 9:19 PM, Dmitry Torokhov wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi Andrew,
>
> On Wed, Oct 23, 2019 at 01:24:05AM +0000, Andrew Duggan wrote:
>> A bug in hid-rmi was causing rmi_unregister_transport_device() to be
>> called even if the call to rmi_register_transport_device() failed to
>> allocate the rmi device. A patch has been submitted to fix the issue in
>> hid-rmi. This patch will ensure that should a simialr situation
>> occur then the rmi driver will not dereference a NULL pointer.
> This looks like "garbage in, garbage out" problem where we should not be
> calling unregister in the first place. I'd rather not apply this.
That's fine, like I said the actual fix to prevent
rmi_unregister_transport_device() from being called inappropriately is
in the hid-rmi driver.
https://lore.kernel.org/linux-input/[email protected]/
I was on the fence on whether or not it was better to prevent the NULL
pointer dereference even at the expense of masking bugs like the one in
the hid-rmi driver.
Thanks for the feedback,
Andrew
> Thanks.
>
> --
> Dmitry
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,
--
Jiri Kosina
SUSE Labs
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/[email protected]/
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,
--
Jiri Kosina
SUSE Labs