diff mbox series

[1/4] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device

Message ID 20190212120523.27463-1-hong.liu@intel.com (mailing list archive)
State Mainlined
Commit 0d28f49412405d87d3aae83da255070a46e67627
Delegated to: Jiri Kosina
Headers show
Series [1/4] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device | expand

Commit Message

Hong Liu Feb. 12, 2019, 12:05 p.m. UTC 
When performing a warm reset in ishtp bus driver, the ishtp_cl_device
will not be removed, its fw_client still points to the already freed
ishtp_device.fw_clients array.

Later after driver finishing ishtp client enumeration, this dangling
pointer may cause driver to bind the wrong ishtp_cl_device to the new
client, causing wrong callback to be called for messages intended for
the new client.

This helps in development of firmware where frequent switching of
firmwares is required without Linux reboot.

Signed-off-by: Hong Liu <hong.liu@intel.com>
Tested-by: Hongyan Song <hongyan.song@intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jiri Kosina Feb. 13, 2019, 10:56 p.m. UTC | #1 
On Tue, 12 Feb 2019, Hong Liu wrote:

> When performing a warm reset in ishtp bus driver, the ishtp_cl_device
> will not be removed, its fw_client still points to the already freed
> ishtp_device.fw_clients array.
> 
> Later after driver finishing ishtp client enumeration, this dangling
> pointer may cause driver to bind the wrong ishtp_cl_device to the new
> client, causing wrong callback to be called for messages intended for
> the new client.
> 
> This helps in development of firmware where frequent switching of
> firmwares is required without Linux reboot.
> 
> Signed-off-by: Hong Liu <hong.liu@intel.com>
> Tested-by: Hongyan Song <hongyan.song@intel.com>

Srinivas, could you please ack this series? Thanks.

> ---
>  drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index 728dc6d4561a..a271d6d169b1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
>  	spin_lock_irqsave(&cl->dev->device_list_lock, flags);
>  	list_for_each_entry(cl_device, &cl->dev->device_list,
>  			device_link) {
> -		if (cl_device->fw_client->client_id == cl->fw_client_id) {
> +		if (cl_device->fw_client &&
> +		    cl_device->fw_client->client_id == cl->fw_client_id) {
>  			cl->device = cl_device;
>  			rv = 0;
>  			break;
> @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
>  	spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
>  	list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
>  				 device_link) {
> +		cl_device->fw_client = NULL;
>  		if (warm_reset && cl_device->reference_count)
>  			continue;
>  
> -- 
> 2.20.1
>
srinivas pandruvada Feb. 15, 2019, 1:46 p.m. UTC | #2 
On Tue, 2019-02-12 at 20:05 +0800, Hong Liu wrote:
> When performing a warm reset in ishtp bus driver, the ishtp_cl_device
> will not be removed, its fw_client still points to the already freed
> ishtp_device.fw_clients array.
> 
> Later after driver finishing ishtp client enumeration, this dangling
> pointer may cause driver to bind the wrong ishtp_cl_device to the new
> client, causing wrong callback to be called for messages intended for
> the new client.
> 
> This helps in development of firmware where frequent switching of
> firmwares is required without Linux reboot.
> 
> Signed-off-by: Hong Liu <hong.liu@intel.com>
> Tested-by: Hongyan Song <hongyan.song@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index 728dc6d4561a..a271d6d169b1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
>  	spin_lock_irqsave(&cl->dev->device_list_lock, flags);
>  	list_for_each_entry(cl_device, &cl->dev->device_list,
>  			device_link) {
> -		if (cl_device->fw_client->client_id == cl-
> >fw_client_id) {
> +		if (cl_device->fw_client &&
> +		    cl_device->fw_client->client_id == cl-
> >fw_client_id) {
>  			cl->device = cl_device;
>  			rv = 0;
>  			break;
> @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct
> ishtp_device *ishtp_dev,
>  	spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
>  	list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
>  				 device_link) {
> +		cl_device->fw_client = NULL;
>  		if (warm_reset && cl_device->reference_count)
>  			continue;
>
srinivas pandruvada Feb. 15, 2019, 1:49 p.m. UTC | #3 
On Wed, 2019-02-13 at 23:56 +0100, Jiri Kosina wrote:
> On Tue, 12 Feb 2019, Hong Liu wrote:
> 
> > When performing a warm reset in ishtp bus driver, the
> > ishtp_cl_device
> > will not be removed, its fw_client still points to the already
> > freed
> > ishtp_device.fw_clients array.
> > 
> > Later after driver finishing ishtp client enumeration, this
> > dangling
> > pointer may cause driver to bind the wrong ishtp_cl_device to the
> > new
> > client, causing wrong callback to be called for messages intended
> > for
> > the new client.
> > 
> > This helps in development of firmware where frequent switching of
> > firmwares is required without Linux reboot.
> > 
> > Signed-off-by: Hong Liu <hong.liu@intel.com>
> > Tested-by: Hongyan Song <hongyan.song@intel.com>
> 
> Srinivas, could you please ack this series? Thanks.
Just did. I looked at these patches before during our internal reviews.

Thanks,
Srinivas

> 
> > ---
> >  drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> > b/drivers/hid/intel-ish-hid/ishtp/bus.c
> > index 728dc6d4561a..a271d6d169b1 100644
> > --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> > +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> > @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl)
> >  	spin_lock_irqsave(&cl->dev->device_list_lock, flags);
> >  	list_for_each_entry(cl_device, &cl->dev->device_list,
> >  			device_link) {
> > -		if (cl_device->fw_client->client_id == cl-
> > >fw_client_id) {
> > +		if (cl_device->fw_client &&
> > +		    cl_device->fw_client->client_id == cl-
> > >fw_client_id) {
> >  			cl->device = cl_device;
> >  			rv = 0;
> >  			break;
> > @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct
> > ishtp_device *ishtp_dev,
> >  	spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
> >  	list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
> >  				 device_link) {
> > +		cl_device->fw_client = NULL;
> >  		if (warm_reset && cl_device->reference_count)
> >  			continue;
> >  
> > -- 
> > 2.20.1
> > 
> 
>
Jiri Kosina Feb. 15, 2019, 9:49 p.m. UTC | #4 
I've now applied the whole series. Thanks,
diff mbox series

Patch

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 728dc6d4561a..a271d6d169b1 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -675,7 +675,8 @@  int ishtp_cl_device_bind(struct ishtp_cl *cl)
 	spin_lock_irqsave(&cl->dev->device_list_lock, flags);
 	list_for_each_entry(cl_device, &cl->dev->device_list,
 			device_link) {
-		if (cl_device->fw_client->client_id == cl->fw_client_id) {
+		if (cl_device->fw_client &&
+		    cl_device->fw_client->client_id == cl->fw_client_id) {
 			cl->device = cl_device;
 			rv = 0;
 			break;
@@ -735,6 +736,7 @@  void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
 	spin_lock_irqsave(&ishtp_dev->device_list_lock, flags);
 	list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list,
 				 device_link) {
+		cl_device->fw_client = NULL;
 		if (warm_reset && cl_device->reference_count)
 			continue;