[PATCHES] HID: huion: Fix "doing dma on the stack"

[PATCHES] HID: huion: Fix "doing dma on the stack"

[Nikolai Kondrashov]

Hi everyone,

These two patches work towards fixing the "doing dma on the stack" reported by
a static analyzer (I'm not sure which one) for my recent changes to hid-huion
driver.

The first one is easier to read with whitespace changes ingored (-w).

Both patches were tested by me with a Huion H610N tablet.

Nick

 drivers/hid/hid-huion.c | 128 +++++++++++++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 50 deletions(-)

[PATCH 1/2] HID: huion: Fail on parameter retrieval errors

[Nikolai Kondrashov]

Fail Huion tablet interface enabling and probing, if parameter retrieval
fails. Move the main code path out of the else block accordingly.

This should prevent devices appearing in a half-working state due to
original report descriptor being used, simplifying diagnostics. This
also makes it easier to add cleanup in later commits, as error handling
is simplified.
---
 drivers/hid/hid-huion.c | 96 +++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index 60f44cd..a683d4b 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -116,6 +116,10 @@ static int huion_tablet_enable(struct hid_device *hdev)
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
 	struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
 	__le16 buf[6];
+	s32 params[HUION_PH_ID_NUM];
+	s32 resolution;
+	__u8 *p;
+	s32 v;
 
 	/*
 	 * Read string descriptor containing tablet parameters. The specific
@@ -128,56 +132,54 @@ static int huion_tablet_enable(struct hid_device *hdev)
 				(USB_DT_STRING << 8) + 0x64,
 				0x0409, buf, sizeof(buf),
 				USB_CTRL_GET_TIMEOUT);
-	if (rc == -EPIPE)
-		hid_warn(hdev, "device parameters not found\n");
-	else if (rc < 0)
-		hid_warn(hdev, "failed to get device parameters: %d\n", rc);
-	else if (rc != sizeof(buf))
-		hid_warn(hdev, "invalid device parameters\n");
-	else {
-		s32 params[HUION_PH_ID_NUM];
-		s32 resolution;
-		__u8 *p;
-		s32 v;
+	if (rc == -EPIPE) {
+		hid_err(hdev, "device parameters not found\n");
+		return -ENODEV;
+	} else if (rc < 0) {
+		hid_err(hdev, "failed to get device parameters: %d\n", rc);
+		return -ENODEV;
+	} else if (rc != sizeof(buf)) {
+		hid_err(hdev, "invalid device parameters\n");
+		return -ENODEV;
+	}
 
-		/* Extract device parameters */
-		params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
-		params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
-		params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
-		resolution = le16_to_cpu(buf[5]);
-		if (resolution == 0) {
-			params[HUION_PH_ID_X_PM] = 0;
-			params[HUION_PH_ID_Y_PM] = 0;
-		} else {
-			params[HUION_PH_ID_X_PM] = params[HUION_PH_ID_X_LM] *
-							1000 / resolution;
-			params[HUION_PH_ID_Y_PM] = params[HUION_PH_ID_Y_LM] *
-							1000 / resolution;
-		}
+	/* Extract device parameters */
+	params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
+	params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
+	params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
+	resolution = le16_to_cpu(buf[5]);
+	if (resolution == 0) {
+		params[HUION_PH_ID_X_PM] = 0;
+		params[HUION_PH_ID_Y_PM] = 0;
+	} else {
+		params[HUION_PH_ID_X_PM] = params[HUION_PH_ID_X_LM] *
+						1000 / resolution;
+		params[HUION_PH_ID_Y_PM] = params[HUION_PH_ID_Y_LM] *
+						1000 / resolution;
+	}
 
-		/* Allocate fixed report descriptor */
-		drvdata->rdesc = devm_kmalloc(&hdev->dev,
-					sizeof(huion_tablet_rdesc_template),
-					GFP_KERNEL);
-		if (drvdata->rdesc == NULL) {
-			hid_err(hdev, "failed to allocate fixed rdesc\n");
-			return -ENOMEM;
-		}
-		drvdata->rsize = sizeof(huion_tablet_rdesc_template);
+	/* Allocate fixed report descriptor */
+	drvdata->rdesc = devm_kmalloc(&hdev->dev,
+				sizeof(huion_tablet_rdesc_template),
+				GFP_KERNEL);
+	if (drvdata->rdesc == NULL) {
+		hid_err(hdev, "failed to allocate fixed rdesc\n");
+		return -ENOMEM;
+	}
+	drvdata->rsize = sizeof(huion_tablet_rdesc_template);
 
-		/* Format fixed report descriptor */
-		memcpy(drvdata->rdesc, huion_tablet_rdesc_template,
-			drvdata->rsize);
-		for (p = drvdata->rdesc;
-		     p <= drvdata->rdesc + drvdata->rsize - 4;) {
-			if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
-			    p[3] < sizeof(params)) {
-				v = params[p[3]];
-				put_unaligned(cpu_to_le32(v), (s32 *)p);
-				p += 4;
-			} else {
-				p++;
-			}
+	/* Format fixed report descriptor */
+	memcpy(drvdata->rdesc, huion_tablet_rdesc_template,
+		drvdata->rsize);
+	for (p = drvdata->rdesc;
+	     p <= drvdata->rdesc + drvdata->rsize - 4;) {
+		if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
+		    p[3] < sizeof(params)) {
+			v = params[p[3]];
+			put_unaligned(cpu_to_le32(v), (s32 *)p);
+			p += 4;
+		} else {
+			p++;
 		}
 	}
 
-- 
2.0.1

[PATCH 2/2] HID: huion: Use allocated buffer for DMA

[Nikolai Kondrashov]

Allocate a buffer with kmalloc for receiving the parameters string
descriptor with usb_control_msg, instead of using a buffer on the stack,
as the latter is unsafe. Use an enum for indices into the buffer to
ensure the buffer size if sufficient.

This fixes the static checker error "doing dma on the stack (buf)".
---
 drivers/hid/hid-huion.c | 50 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index a683d4b..61b68ca 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -84,6 +84,15 @@ static const __u8 huion_tablet_rdesc_template[] = {
 	0xC0                    /*  End Collection                          */
 };
 
+/* Parameter indices */
+enum huion_prm {
+	HUION_PRM_X_LM		= 1,
+	HUION_PRM_Y_LM		= 2,
+	HUION_PRM_PRESSURE_LM	= 4,
+	HUION_PRM_RESOLUTION	= 5,
+	HUION_PRM_NUM
+};
+
 /* Driver data */
 struct huion_drvdata {
 	__u8 *rdesc;
@@ -115,7 +124,8 @@ static int huion_tablet_enable(struct hid_device *hdev)
 	int rc;
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
 	struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
-	__le16 buf[6];
+	__le16 *buf = NULL;
+	size_t len;
 	s32 params[HUION_PH_ID_NUM];
 	s32 resolution;
 	__u8 *p;
@@ -127,27 +137,38 @@ static int huion_tablet_enable(struct hid_device *hdev)
 	 * driver traffic.
 	 * NOTE: This enables fully-functional tablet mode.
 	 */
+	len = HUION_PRM_NUM * sizeof(*buf);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (buf == NULL) {
+		hid_err(hdev, "failed to allocate parameter buffer\n");
+		rc = -ENOMEM;
+		goto cleanup;
+	}
 	rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
 				USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
 				(USB_DT_STRING << 8) + 0x64,
-				0x0409, buf, sizeof(buf),
+				0x0409, buf, len,
 				USB_CTRL_GET_TIMEOUT);
 	if (rc == -EPIPE) {
 		hid_err(hdev, "device parameters not found\n");
-		return -ENODEV;
+		rc = -ENODEV;
+		goto cleanup;
 	} else if (rc < 0) {
 		hid_err(hdev, "failed to get device parameters: %d\n", rc);
-		return -ENODEV;
-	} else if (rc != sizeof(buf)) {
+		rc = -ENODEV;
+		goto cleanup;
+	} else if (rc != len) {
 		hid_err(hdev, "invalid device parameters\n");
-		return -ENODEV;
+		rc = -ENODEV;
+		goto cleanup;
 	}
 
 	/* Extract device parameters */
-	params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
-	params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
-	params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
-	resolution = le16_to_cpu(buf[5]);
+	params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[HUION_PRM_X_LM]);
+	params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[HUION_PRM_Y_LM]);
+	params[HUION_PH_ID_PRESSURE_LM] =
+		le16_to_cpu(buf[HUION_PRM_PRESSURE_LM]);
+	resolution = le16_to_cpu(buf[HUION_PRM_RESOLUTION]);
 	if (resolution == 0) {
 		params[HUION_PH_ID_X_PM] = 0;
 		params[HUION_PH_ID_Y_PM] = 0;
@@ -164,7 +185,8 @@ static int huion_tablet_enable(struct hid_device *hdev)
 				GFP_KERNEL);
 	if (drvdata->rdesc == NULL) {
 		hid_err(hdev, "failed to allocate fixed rdesc\n");
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto cleanup;
 	}
 	drvdata->rsize = sizeof(huion_tablet_rdesc_template);
 
@@ -183,7 +205,11 @@ static int huion_tablet_enable(struct hid_device *hdev)
 		}
 	}
 
-	return 0;
+	rc = 0;
+
+cleanup:
+	kfree(buf);
+	return rc;
 }
 
 static int huion_probe(struct hid_device *hdev, const struct hid_device_id *id)
-- 
2.0.1

Re: [PATCHES] HID: huion: Fix "doing dma on the stack"

[Jiri Kosina]

On Mon, 11 Aug 2014, Nikolai Kondrashov wrote:

> Hi everyone,
> 
> These two patches work towards fixing the "doing dma on the stack" reported by
> a static analyzer (I'm not sure which one) for my recent changes to hid-huion
> driver.
> 
> The first one is easier to read with whitespace changes ingored (-w).
> 
> Both patches were tested by me with a Huion H610N tablet.
> 
> Nick

Nikolai,

thanks, I will be reviewing the patches soon and queuing for 3.17 still 
eventually.

They seem to be missing your signoff though ... is that on purpose? 
Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCHES] HID: huion: Fix "doing dma on the stack"

[Nikolai Kondrashov]

On 08/12/2014 12:27 PM, Jiri Kosina wrote:
> On Mon, 11 Aug 2014, Nikolai Kondrashov wrote:
>
>> Hi everyone,
>>
>> These two patches work towards fixing the "doing dma on the stack" reported by
>> a static analyzer (I'm not sure which one) for my recent changes to hid-huion
>> driver.
>>
>> The first one is easier to read with whitespace changes ingored (-w).
>>
>> Both patches were tested by me with a Huion H610N tablet.
>>
>> Nick
>
> Nikolai,
>
> thanks, I will be reviewing the patches soon and queuing for 3.17 still
> eventually.

Thanks, Jiri.

> They seem to be missing your signoff though ... is that on purpose?

No, I forgot it, sorry. Would you like me to re-send them with my signoff?

Otherwise:

Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>

Nick

Re: [PATCHES] HID: huion: Fix "doing dma on the stack"

[Jiri Kosina]

On Tue, 12 Aug 2014, Nikolai Kondrashov wrote:

> > They seem to be missing your signoff though ... is that on purpose?
> 
> No, I forgot it, sorry. Would you like me to re-send them with my signoff?
> 
> Otherwise:
> 
> Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>

No need to resend; I've applied the patches now with the signoff added. 
Thanks,

-- 
Jiri Kosina
SUSE Labs