[PATCH] atmel: fix an error handle in mxt_probe

[PATCH] atmel: fix an error handle in mxt_probe

[Pan Xinhui]

mxt_probe() may fail at last step, and the queue_work scheduled by request_firmware_nowait
may run later and then access some data which is freed.
To handle this error, add one mutex_lock to cover such case. It may cause module load delay only when the probe fails.

here is the detail.

module load:                                                             worker_thread:
mxt_probe -> mxt_initialize -> request_firmware_nowait (schedule_work)
                    |
             sysfs_create_group (fails)                                    mxt_config_cb -> mxt_configure_objects (may access data freed)
                    |
             err_free_object: some cleanup work, like free(data).

Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
---
  drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2875ddf..af057c0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1978,10 +1978,19 @@ err_free_mem:
  static int mxt_configure_objects(struct mxt_data *data,
  				 const struct firmware *cfg);
  
+static DEFINE_MUTEX(err_probe_lock);
+static int err_probe;
+
  static void mxt_config_cb(const struct firmware *cfg, void *ctx)
  {
+	mutex_lock(&err_probe_lock);
+	if (err_probe) {
+		mutex_unlock(&err_probe_lock);
+		return;
+	}
  	mxt_configure_objects(ctx, cfg);
  	release_firmware(cfg);
+	mutex_unlock(&err_probe_lock);
  }
  
  static int mxt_initialize(struct mxt_data *data)
@@ -2423,6 +2432,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
  	const struct mxt_platform_data *pdata;
  	int error;
  
+	err_probe = 0;
+
  	pdata = dev_get_platdata(&client->dev);
  	if (!pdata) {
  		pdata = mxt_parse_dt(client);
@@ -2472,6 +2483,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
  	return 0;
  
  err_free_object:
+	mutex_lock(&err_probe_lock);
+	err_probe = -1;
+	mutex_unlock(&err_probe_lock);
  	mxt_free_input_device(data);
  	mxt_free_object_table(data);
  err_free_irq:
-- 
1.9.1

Re: [PATCH] atmel: fix an error handle in mxt_probe

[Dmitry Torokhov]

Hi,

On Wed, Apr 22, 2015 at 06:46:58PM +0800, Pan Xinhui wrote:
> mxt_probe() may fail at last step, and the queue_work scheduled by request_firmware_nowait
> may run later and then access some data which is freed.
> To handle this error, add one mutex_lock to cover such case. It may cause module load delay only when the probe fails.
> 
> here is the detail.
> 
> module load:                                                             worker_thread:
> mxt_probe -> mxt_initialize -> request_firmware_nowait (schedule_work)
>                    |
>             sysfs_create_group (fails)                                    mxt_config_cb -> mxt_configure_objects (may access data freed)
>                    |
>             err_free_object: some cleanup work, like free(data).
> 
> Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 2875ddf..af057c0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1978,10 +1978,19 @@ err_free_mem:
>  static int mxt_configure_objects(struct mxt_data *data,
>  				 const struct firmware *cfg);
> +static DEFINE_MUTEX(err_probe_lock);
> +static int err_probe;

While you are right that bad things will happen if we let
request_firmware_nowait() run after driver fails to bind to the device
using statics to indicate success or failure is not good idea since you
may have several such devices in your unit. Also it still doe snot help
if you decide to unbind the device quickly or unlock the module.

I guess the best way is to signal a completion from callback and wait
for it in error path and in remove().

Thanks.

> +
>  static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>  {
> +	mutex_lock(&err_probe_lock);
> +	if (err_probe) {
> +		mutex_unlock(&err_probe_lock);
> +		return;
> +	}
>  	mxt_configure_objects(ctx, cfg);
>  	release_firmware(cfg);
> +	mutex_unlock(&err_probe_lock);
>  }
>  static int mxt_initialize(struct mxt_data *data)
> @@ -2423,6 +2432,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	const struct mxt_platform_data *pdata;
>  	int error;
> +	err_probe = 0;
> +
>  	pdata = dev_get_platdata(&client->dev);
>  	if (!pdata) {
>  		pdata = mxt_parse_dt(client);
> @@ -2472,6 +2483,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	return 0;
>  err_free_object:
> +	mutex_lock(&err_probe_lock);
> +	err_probe = -1;
> +	mutex_unlock(&err_probe_lock);
>  	mxt_free_input_device(data);
>  	mxt_free_object_table(data);
>  err_free_irq:
> -- 
> 1.9.1

-- 
Dmitry

Re: [PATCH] atmel: fix an error handle in mxt_probe

[Pan Xinhui]

HI, Dmitry
	thanks for your reply :)

On 2015年05月14日 01:41, Dmitry Torokhov wrote:
> Hi,
>
> On Wed, Apr 22, 2015 at 06:46:58PM +0800, Pan Xinhui wrote:
>> mxt_probe() may fail at last step, and the queue_work scheduled by request_firmware_nowait
>> may run later and then access some data which is freed.
>> To handle this error, add one mutex_lock to cover such case. It may cause module load delay only when the probe fails.
>>
>> here is the detail.
>>
>> module load:                                                             worker_thread:
>> mxt_probe -> mxt_initialize -> request_firmware_nowait (schedule_work)
>>                     |
>>              sysfs_create_group (fails)                                    mxt_config_cb -> mxt_configure_objects (may access data freed)
>>                     |
>>              err_free_object: some cleanup work, like free(data).
>>
>> Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
>> ---
>>   drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 2875ddf..af057c0 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -1978,10 +1978,19 @@ err_free_mem:
>>   static int mxt_configure_objects(struct mxt_data *data,
>>   				 const struct firmware *cfg);
>> +static DEFINE_MUTEX(err_probe_lock);
>> +static int err_probe;
>
> While you are right that bad things will happen if we let
> request_firmware_nowait() run after driver fails to bind to the device
> using statics to indicate success or failure is not good idea since you
> may have several such devices in your unit. Also it still doe snot help
> if you decide to unbind the device quickly or unlock the module.
>
> I guess the best way is to signal a completion from callback and wait
> for it in error path and in remove().
>
> Thanks.
>

yes, statics is not good.
It's also a good point that do some work both in err patch and in_remove().

thanks for your nice advices. I will work out patch V2.

thanks

xinhui

>> +
>>   static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>>   {
>> +	mutex_lock(&err_probe_lock);
>> +	if (err_probe) {
>> +		mutex_unlock(&err_probe_lock);
>> +		return;
>> +	}
>>   	mxt_configure_objects(ctx, cfg);
>>   	release_firmware(cfg);
>> +	mutex_unlock(&err_probe_lock);
>>   }
>>   static int mxt_initialize(struct mxt_data *data)
>> @@ -2423,6 +2432,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   	const struct mxt_platform_data *pdata;
>>   	int error;
>> +	err_probe = 0;
>> +
>>   	pdata = dev_get_platdata(&client->dev);
>>   	if (!pdata) {
>>   		pdata = mxt_parse_dt(client);
>> @@ -2472,6 +2483,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   	return 0;
>>   err_free_object:
>> +	mutex_lock(&err_probe_lock);
>> +	err_probe = -1;
>> +	mutex_unlock(&err_probe_lock);
>>   	mxt_free_input_device(data);
>>   	mxt_free_object_table(data);
>>   err_free_irq:
>> --
>> 1.9.1
>