Bug 217169 - BUG: binary data included in HID-SENSOR device name taints ftrace
Summary: BUG: binary data included in HID-SENSOR device name taints ftrace
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_input-devices
URL:
Keywords:
Depends on:
Blocks: 178231
  Show dependency tree
 
Reported: 2023-03-09 23:43 UTC by Todd Brandt
Modified: 2023-03-11 00:11 UTC (History)
1 user (show)

See Also:
Kernel Version: 6.3.0-rc1
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Todd Brandt 2023-03-09 23:43:18 UTC
I've run into an issue in 6.3.0-rc1 that causes problems with
ftrace and I've bisected it to this commit:

commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD, refs/bisect/bad)
Author: Philipp Jungkamp <p.jungkamp@gmx.net>
Date:   Fri Nov 25 00:38:38 2022 +0100

    HID: hid-sensor-custom: Allow more custom iio sensors
    
    The known LUID table for established/known custom HID sensors was
    limited to sensors with "INTEL" as manufacturer. But some vendors such
    as Lenovo also include fairly standard iio sensors (e.g. ambient light)
    in their custom sensors.
    
    Expand the known custom sensors table by a tag used for the platform
    device name and match sensors based on the LUID as well as optionally
    on model and manufacturer properties.
    
    Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
    Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
    Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
    Signed-off-by: Jiri Kosina <jkosina@suse.cz>

You're using raw binary data as part of the devname in the "real_usage"
string, but it includes chars other than ASCII, and those chars end 
up being printed out in the ftrace log which is meant to be ASCII only.

-       /* HID-SENSOR-INT-REAL_USAGE_ID */
-       dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
+       /* HID-SENSOR-TAG-REAL_USAGE_ID */
+       dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
+                            match->tag, real_usage);

My sleepgraph tool started to crash because it read these lines from ftrace:

device_pm_callback_start: platform HID-SENSOR-INT-020b�.39.auto, parent: 001F:8087:0AC2.0003, [suspend]
device_pm_callback_end: platform HID-SENSOR-INT-020b�.39.auto, err=0

The "HID-SENSOR-INT-020b�.39.auto" string includes a binary char that kills
python3 code that loops through an ascii file as such:

  File "/usr/bin/sleepgraph", line 5579, in executeSuspend
    for line in fp:
  File "/usr/lib/python3.10/codecs.py", line 322, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 1568: invalid start byte

I've updated sleepgraph to handle random non-ascii chars, but other tools
may suffer the same fate.

In theory all you have to do to fix this is update this section of code to filter out non-ascii characters as well as convert them to lower case:

file: drivers/hid/hid-sensor-custom.c
func: hid_sensor_register_platform_device

/* usage id are all lowcase */
for (c = real_usage; *c != '\0'; c++)
	*c = tolower(*c);
Comment 1 Todd Brandt 2023-03-11 00:11:07 UTC
I spoke with Phillip and he came up with a fix that works. I've posted it here:

https://marc.info/?l=linux-iio&m=167849244508779&w=2

I've tested it and it fixes the issue. The problem was with a buffer overrun because the destination string wasn't initialized to 0. It's 5 bytes long with 4 bytes copied and the code expected the 5th byte to be a null char.

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 3e3f89e01d81..d85398721659 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct platform_device *pdev,
 				    struct hid_sensor_hub_device *hsdev,
 				    const struct hid_sensor_custom_match *match)
 {
-	char real_usage[HID_SENSOR_USAGE_LENGTH];
+	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
 	struct platform_device *custom_pdev;
 	const char *dev_name;
 	char *c;

Note You need to log in before you can comment on or make changes to this bug.