[PATCH] HID: ft260: improve error handling of...

[PATCH] HID: ft260: improve error handling of ft260_hid_feature_report_get()

From: trix@redhat.com
Date: 2021-05-07 18:38:09

From: Tom Rix <trix@redhat.com>

Static analysis reports this representative problem

hid-ft260.c:787:9: warning: 4th function call argument is an
  uninitialized value
        return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Uses of ft260_hid_feature_report_get() check if the return size matches
the requested size.  But the function can also fail with at least -ENOMEM.
Add the < 0 checks.

In ft260_hid_feature_report_get(), do not do the memcpy to the caller's
buffer if there is an error.

Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/hid/hid-ft260.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 7a9ba984a75a..628fa664a10b 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c

@@ -249,7 +249,8 @@ static int ft260_hid_feature_report_get(struct hid_device *hdev,
 
 	ret = hid_hw_raw_request(hdev, report_id, buf, len, HID_FEATURE_REPORT,
 				 HID_REQ_GET_REPORT);
-	memcpy(data, buf, len);
+	if (ret == len)
+		memcpy(data, buf, len);
 	kfree(buf);
 	return ret;
 }

@@ -295,12 +296,16 @@ static int ft260_xfer_status(struct ft260_device *dev)
 	struct hid_device *hdev = dev->hdev;
 	struct ft260_get_i2c_status_report report;
 	int ret;
+	int len = sizeof(report);
 
 	ret = ft260_hid_feature_report_get(hdev, FT260_I2C_STATUS,
-					   (u8 *)&report, sizeof(report));
-	if (ret < 0) {
+					   (u8 *)&report, len);
+	if (ret != len) {
 		hid_err(hdev, "failed to retrieve status: %d\n", ret);
-		return ret;
+		if (ret >= 0)
+			return -EIO;
+		else
+			return ret;
 	}
 
 	dev->clock = le16_to_cpu(report.clock);

@@ -728,6 +733,8 @@ static int ft260_get_system_config(struct hid_device *hdev,
 		hid_err(hdev, "failed to retrieve system status\n");
 		if (ret >= 0)
 			return -EIO;
+		else
+			return ret;
 	}
 	return 0;
 }

@@ -782,6 +789,8 @@ static int ft260_byte_show(struct hid_device *hdev, int id, u8 *cfg, int len,
 	ret = ft260_hid_feature_report_get(hdev, id, cfg, len);
 	if (ret != len && ret >= 0)
 		return -EIO;
+	else if (ret < 0)
+		return  ret;
 
 	return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
 }

@@ -794,6 +803,8 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
 	ret = ft260_hid_feature_report_get(hdev, id, cfg, len);
 	if (ret != len && ret >= 0)
 		return -EIO;
+	else if (ret < 0)
+		return ret;
 
 	return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
 }

-- 
2.26.3

Re: [PATCH] HID: ft260: improve error handling of ft260_hid_feature_report_get()

From: Michael Zaidman <michael.zaidman@gmail.com>
Date: 2021-05-08 19:39:50

On Fri, May 07, 2021 at 11:37:57AM -0700, trix@redhat.com wrote:

From: Tom Rix <trix@redhat.com>

Static analysis reports this representative problem

hid-ft260.c:787:9: warning: 4th function call argument is an
  uninitialized value
        return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Uses of ft260_hid_feature_report_get() check if the return size matches
the requested size.  But the function can also fail with at least -ENOMEM.
Add the < 0 checks.

Hi Tom, thanks for catching and fixing it!

I applied the patch, built the driver, and run some tests on my HW setup -
no regression so far. But I think the fix can be improved even more, by
reducing the number of questions to one in the successful case where the
performance matters. Please see the proposal below inline.

quoted hunk

In ft260_hid_feature_report_get(), do not do the memcpy to the caller's
buffer if there is an error.

Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/hid/hid-ft260.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 7a9ba984a75a..628fa664a10b 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c

@@ -249,7 +249,8 @@ static int ft260_hid_feature_report_get(struct hid_device *hdev,
 
 	ret = hid_hw_raw_request(hdev, report_id, buf, len, HID_FEATURE_REPORT,
 				 HID_REQ_GET_REPORT);
-	memcpy(data, buf, len);
+	if (ret == len)
+		memcpy(data, buf, len);
 	kfree(buf);
 	return ret;
 }

@@ -295,12 +296,16 @@ static int ft260_xfer_status(struct ft260_device *dev)
 	struct hid_device *hdev = dev->hdev;
 	struct ft260_get_i2c_status_report report;
 	int ret;
+	int len = sizeof(report);
 
 	ret = ft260_hid_feature_report_get(hdev, FT260_I2C_STATUS,
-					   (u8 *)&report, sizeof(report));
-	if (ret < 0) {
+					   (u8 *)&report, len);
+	if (ret != len) {
 		hid_err(hdev, "failed to retrieve status: %d\n", ret);
-		return ret;
+		if (ret >= 0)
+			return -EIO;
+		else
+			return ret;
 	}
 
 	dev->clock = le16_to_cpu(report.clock);

@@ -728,6 +733,8 @@ static int ft260_get_system_config(struct hid_device *hdev,
 		hid_err(hdev, "failed to retrieve system status\n");
 		if (ret >= 0)
 			return -EIO;
+		else
+			return ret;
 	}
 	return 0;
 }

@@ -782,6 +789,8 @@ static int ft260_byte_show(struct hid_device *hdev, int id, u8 *cfg, int len,
 	ret = ft260_hid_feature_report_get(hdev, id, cfg, len);
 	if (ret != len && ret >= 0)
 		return -EIO;
+	else if (ret < 0)
+		return  ret;

Please consider the below code to reduce the number of questions to one in the "likely" case
and two in the worst-case scenario.

	if (ret != len) {
	        if (ret >= 0)
                        return -EIO;
                else
                        return ret;
        }

quoted hunk

 
 	return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
 }

@@ -794,6 +803,8 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
 	ret = ft260_hid_feature_report_get(hdev, id, cfg, len);
 	if (ret != len && ret >= 0)
 		return -EIO;
+	else if (ret < 0)
+		return ret;

The same.

 
 	return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
 }
-- 
2.26.3

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help