atmel: fix an error handle in mxt_probe

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 <[email protected]>

---
 drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

drivers/input/touchscreen/atmel_mxt_ts.c +0 -0

index 2875ddf..af057c0 100644

drivers/input/touchscreen/atmel_mxt_ts.c +12 -0

+
 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:

Signature · 1 line

1.9.1

Hi,

Quoted 23 lines · 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 <[email protected]>

---
  drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

drivers/input/touchscreen/atmel_mxt_ts.c +0 -0

index 2875ddf..af057c0 100644

drivers/input/touchscreen/atmel_mxt_ts.c err_free_mem: · line 1981

Quoted 4 lines

drivers/input/touchscreen/atmel_mxt_ts.c +2 -0

@@ -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.

Quoted 32 lines

drivers/input/touchscreen/atmel_mxt_ts.c +12 -1

+
  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