Re: [PATCH 6/8] Input: psmouse - add support for SMBus companions

[Dmitry Torokhov]

On Fri, Mar 10, 2017 at 06:55:09PM +0100, Benjamin Tissoires wrote:
> On Mar 09 2017 or thereabouts, Dmitry Torokhov wrote:
> > From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > 
> > This provides glue between PS/2 devices that enumerate the RMI4 devices
> > and Elan touchpads to the RMI4 (or Elan) SMBus driver.
> > 
> > The SMBus devices keep their PS/2 connection alive. If the initialization
> > process goes too far (psmouse_activate called), the device disconnects
> > from the I2C bus and stays on the PS/2 bus, that is why we explicitly
> > disable PS/2 device reporting (by calling psmouse_deactivate) before
> > trying to register SMBus companion device.
> > 
> > The HID over I2C devices are enumerated through the ACPI DSDT, and
> > their PS/2 device also exports the InterTouch bit in the extended
> > capability 0x0C. However, the firmware keeps its I2C connection open
> > even after going further in the PS/2 initialization. We don't need
> > to take extra precautions with those device, especially because they
> > block their PS/2 communication when HID over I2C is used.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> >
> 
> Hi Dmitry,
> 
> There is an issue with this patch. The platform_data provided by the
> caller of psmouse_smbus_init() may be dereference, leading to a dangling
> pointer on the stack in rmi4.
> 
> There is no guarantees rmi-smbus will get probed directly and even if it
> does, a later rmmod/modprobe might happen and won't have access to the
> platform data.
> 
> See below for a patch that solves this. Feel free to squash it, change it
> and remove the terrible commit message.
> 
> From a86f766de9c544a8d61da520719287c68a5f1bea Mon Sep 17 00:00:00 2001
> From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> Date: Fri, 10 Mar 2017 18:31:01 +0100
> Subject: [PATCH] Input: smbus companion - store the platform data for later
>  use
> 
> The platform data should be available as long as the i2c_device exists.
> In the current implementation, the platform data is allocated on the
> stack, which gives a dangling pointer to the i2c_client once
> psmouse_smbus_init() ends.
> 
> Duplicate the provided platform data in psmouse_smbus_init() so that
> the allocation/free of pdata is handled by psmouse-smbus.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> ---
>  drivers/input/mouse/psmouse-smbus.c | 18 ++++++++++++++++--
>  drivers/input/mouse/psmouse.h       |  2 +-
>  drivers/input/mouse/synaptics.c     |  5 ++---
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/psmouse-smbus.c b/drivers/input/mouse/psmouse-smbus.c
> index 5bda551..6f603ec 100644
> --- a/drivers/input/mouse/psmouse-smbus.c
> +++ b/drivers/input/mouse/psmouse-smbus.c
> @@ -20,6 +20,7 @@
>  struct psmouse_smbus_dev {
>  	struct psmouse *psmouse;
>  	struct i2c_client *client;
> +	void *pdata;
>  	struct list_head node;
>  	unsigned short addr;
>  	bool dead;
> @@ -166,6 +167,7 @@ static void psmouse_smbus_disconnect(struct psmouse *psmouse)
>  		psmouse_smbus_schedule_remove(smbdev->client);
>  	}
>  
> +	kfree(smbdev->pdata);

We can 't do it here if we fired off a work to unregister i2c_client, as
it may still be referencing it. I guess we can do it in notifier code.
I'll update the patch.

Thanks.

-- 
Dmitry