HID: wacom: Prevent potential null dereference after disconnect

PATCH Superseded Jason Gerecke 2 participants 6 messages Oct 7, 2014 – Oct 9, 2014

Subsystems (2)

HID HID wacom driver

lore

Share & tools

lore

Jason Gerecke

author

2014-10-07 17:54 UTC #0 lore raw

PATCH 1/1 HID: wacom: Prevent potential null dereference after disconnect

Repeated connect/disconnect cycles under GNOME can trigger an occasional OOPS from within e.g. wacom_led_select_store, presumably due to a timing issue where userspace begins setting a value immediately before the device disconnects and our shared data is whisked away.

Signed-off-by Jason Gerecke <[email protected]>

---
 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/hid/wacom_sys.c +12 -1

index 8593047..d9ae467 100644
@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 	unsigned int id;
 	int err;
 
+	if (!wacom)
+		return -ENODEV;
+
 	err = kstrtouint(buf, 10, &id);
 	if (err)
 		return err;

@@ -666,6 +669,8 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
 {									\
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
+	if (!wacom)							\
+		return -ENODEV;						\
 	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
 			 wacom->led.select[SET_ID]);			\
 }									\

@@ -702,7 +707,8 @@ static ssize_t wacom_##name##_luminance_store(struct device *dev,	\
 {									\
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
-									\
+	if (!wacom)							\
+		return -ENODEV;						\
 	return wacom_luminance_store(wacom, &wacom->led.field,		\
 				     buf, count);			\
 }									\

@@ -710,6 +716,8 @@ static ssize_t wacom_##name##_luminance_show(struct device *dev,	\
 	struct device_attribute *attr, char *buf)			\
 {									\
 	struct wacom *wacom = dev_get_drvdata(dev);			\
+	if (!wacom)							\
+		return -ENODEV;
 	return scnprintf(buf, PAGE_SIZE, "%d\n", wacom->led.field);	\
 }									\
 static DEVICE_ATTR(name##_luminance, DEV_ATTR_RW_PERM,			\

@@ -729,6 +737,9 @@ static ssize_t wacom_button_image_store(struct device *dev, int button_id,
 	unsigned len;
 	u8 xfer_id;
 
+	if (!wacom)
+		return -ENODEV;
+
 	if (hdev->bus == BUS_BLUETOOTH) {
 		len = 256;
 		xfer_id = WAC_CMD_ICON_BT_XFER;

Signature · 1 line

2.1.2

Jason Gerecke

author

2014-10-08 18:26 UTC #1 lore raw

PATCH v2 1/1 HID: wacom: Prevent potential null dereference after disconnect

Signed-off-by Jason Gerecke <[email protected]>

---
Changes in v2:
* Added in missing escape character

 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/hid/wacom_sys.c +12 -1

index 8593047..265429b 100644
@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 	unsigned int id;
 	int err;
 
+	if (!wacom)
+		return -ENODEV;
+
 	err = kstrtouint(buf, 10, &id);
 	if (err)
 		return err;

@@ -666,6 +669,8 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
 {									\
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
+	if (!wacom)							\
+		return -ENODEV;						\
 	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
 			 wacom->led.select[SET_ID]);			\
 }									\

@@ -702,7 +707,8 @@ static ssize_t wacom_##name##_luminance_store(struct device *dev,	\
 {									\
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
-									\
+	if (!wacom)							\
+		return -ENODEV;						\
 	return wacom_luminance_store(wacom, &wacom->led.field,		\
 				     buf, count);			\
 }									\

@@ -710,6 +716,8 @@ static ssize_t wacom_##name##_luminance_show(struct device *dev,	\
 	struct device_attribute *attr, char *buf)			\
 {									\
 	struct wacom *wacom = dev_get_drvdata(dev);			\
+	if (!wacom)							\
+		return -ENODEV;						\
 	return scnprintf(buf, PAGE_SIZE, "%d\n", wacom->led.field);	\
 }									\
 static DEVICE_ATTR(name##_luminance, DEV_ATTR_RW_PERM,			\

@@ -729,6 +737,9 @@ static ssize_t wacom_button_image_store(struct device *dev, int button_id,
 	unsigned len;
 	u8 xfer_id;
 
+	if (!wacom)
+		return -ENODEV;
+
 	if (hdev->bus == BUS_BLUETOOTH) {
 		len = 256;
 		xfer_id = WAC_CMD_ICON_BT_XFER;

Signature · 1 line

2.1.2

Dmitry Torokhov

2014-10-08 21:40 UTC #2 lore raw

Quoted 18 lines · Jason Gerecke

Repeated connect/disconnect cycles under GNOME can trigger an occasional OOPS from within e.g. wacom_led_select_store, presumably due to a timing issue where userspace begins setting a value immediately before the device disconnects and our shared data is whisked away.
Signed-off-by Jason Gerecke <[email protected]>
---
Changes in v2:
* Added in missing escape character
 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/hid/wacom_sys.c +0 -0
index 8593047..265429b 100644

drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 646

Quoted 6 lines

drivers/hid/wacom_sys.c +3 -0

@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 	unsigned int id;
 	int err;
 
+	if (!wacom)
+		return -ENODEV;
+

Strong NAK. If device could disappear before this check it could as well disappear after your check.

This patch does not solve anything.

drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 649

Quoted 3 lines

drivers/hid/wacom_sys.c +0 -0

 	err = kstrtouint(buf, 10, &id);
 	if (err)
 		return err;

Thanks.

Signature · 1 line

Dmitry

Jason Gerecke

author

2014-10-09 00:24 UTC #3 lore raw

Quoted 22 lines · Dmitry Torokhov

Quoted 21 lines · Jason Gerecke
Repeated connect/disconnect cycles under GNOME can trigger an occasional OOPS from within e.g. wacom_led_select_store, presumably due to a timing issue where userspace begins setting a value immediately before the device disconnects and our shared data is whisked away.
Signed-off-by Jason Gerecke <[email protected]>
---
Changes in v2:
* Added in missing escape character
 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/hid/wacom_sys.c +0 -0
index 8593047..265429b 100644
@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
      unsigned int id;
      int err;
 

Quoted 9 lines

drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 646
Quoted 3 lines
drivers/hid/wacom_sys.c +3 -0
+     if (!wacom)
+             return -ENODEV;
+
Strong NAK. If device could disappear before this check it could as well disappear after your check.
This patch does not solve anything.

I assume I'll want to either disable interrupts or take a lock depending on if wacom_remove is called from within the interrupt context, but I haven't had to deal with concurrency in the kernel before so I'm not entirely sure which option (or which primitive if locking) would be appropriate...

Quoted 4 lines

drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 649
Quoted 3 lines
drivers/hid/wacom_sys.c +0 -0
      err = kstrtouint(buf, 10, &id);
      if (err)
              return err;

Quoted 8 lines

Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

2014-10-09 00:28 UTC #4 lore raw

Quoted 31 lines · Jason Gerecke

Quoted 29 lines · Dmitry Torokhov
Quoted 18 lines · Jason Gerecke
Repeated connect/disconnect cycles under GNOME can trigger an occasional OOPS from within e.g. wacom_led_select_store, presumably due to a timing issue where userspace begins setting a value immediately before the device disconnects and our shared data is whisked away.
Signed-off-by Jason Gerecke <[email protected]>
---
Changes in v2:
* Added in missing escape character
 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/hid/wacom_sys.c +0 -0
index 8593047..265429b 100644
drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 646
Quoted 6 lines
drivers/hid/wacom_sys.c +3 -0
@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
      unsigned int id;
      int err;
 
+     if (!wacom)
+             return -ENODEV;
+
Strong NAK. If device could disappear before this check it could as well disappear after your check.

Quoted 8 lines

Quoted 2 lines
This patch does not solve anything.
I assume I'll want to either disable interrupts or take a lock depending on if wacom_remove is called from within the interrupt context, but I haven't had to deal with concurrency in the kernel before so I'm not entirely sure which option (or which primitive if locking) would be appropriate...

Actually the sysfs core should not allow anyone descend into sysfs show/store methods once you return from sysfs_remove*(). So you need to make sure that pointer is valid until then.

Thanks.

Signature · 1 line

Dmitry

Jason Gerecke

author

2014-10-09 02:08 UTC #5 lore raw

Quoted 41 lines · Dmitry Torokhov

Quoted 31 lines · Jason Gerecke
Quoted 29 lines · Dmitry Torokhov
Quoted 18 lines · Jason Gerecke
Repeated connect/disconnect cycles under GNOME can trigger an occasional OOPS from within e.g. wacom_led_select_store, presumably due to a timing issue where userspace begins setting a value immediately before the device disconnects and our shared data is whisked away.
Signed-off-by Jason Gerecke <[email protected]>
---
Changes in v2:
* Added in missing escape character
 drivers/hid/wacom_sys.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/hid/wacom_sys.c +0 -0
index 8593047..265429b 100644
drivers/hid/wacom_sys.c static ssize_t wacom_led_select_store(struct device *dev, int set_id, · line 646
Quoted 6 lines
drivers/hid/wacom_sys.c +3 -0
@@ -641,6 +641,9 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
      unsigned int id;
      int err;
 
+     if (!wacom)
+             return -ENODEV;
+
Strong NAK. If device could disappear before this check it could as well disappear after your check.
Quoted 8 lines
Quoted 2 lines
This patch does not solve anything.
I assume I'll want to either disable interrupts or take a lock depending on if wacom_remove is called from within the interrupt context, but I haven't had to deal with concurrency in the kernel before so I'm not entirely sure which option (or which primitive if locking) would be appropriate...

Quoted 8 lines

Actually the sysfs core should not allow anyone descend into sysfs show/store methods once you return from sysfs_remove*(). So you need to make sure that pointer is valid until then.
Thanks.
--
Dmitry

Hmm. That's odd. The wacom_remove function calls wacom_destroy_leds (which is responsible for removing those sysfs nodes) prior to calling wacom_remove_shared_data (which is responsible for freeing that pointer). I could imagine that a disconnect which occurred after the sysfs checks were satisfied but before our function was called would be able to get around that, but I don't know if the kernel can be interrupted while the sysfs write is being handled. I'll double-check what the actual state of things is when the OOPS happens...

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html