Best Regards,
Even Xu

-----Original Message-----
From: Todd Brandt <todd.e.brandt@linux.intel.com> 
Sent: Saturday, March 11, 2023 7:36 AM
To: Philipp Jungkamp <redacted>; srinivas pandruvada
[off-list ref]; linux-input@vger.kernel.org;
linux-kernel@vger.kernel.org; Xu, Even [off-list ref]
Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E
[off-list ref]
Subject: Re: BUG: hid-sensor-ids code includes binary data in device
name

On Fri, 2023-03-10 at 15:35 +0100, Philipp Jungkamp wrote:

quoted

Hello,

on v3 of the patchset I had this comment on the 'real_usage'
initialization:

quoted

quoted

-       char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
+       char real_usage[HID_SENSOR_USAGE_LENGTH];
        struct platform_device *custom_pdev;
        const char *dev_name;
        char *c;

-       /* copy real usage id */
-       memcpy(real_usage, known_sensor_luid[index], 4);
+       memcpy(real_usage, match->luid, 4);
+       real_usage[4] = '\0';

Why the change in approach for setting the NULL character?
Doesn't seem relevant to main purpose of this patch.

Based on the comment, I changed that in the final v4 revision to:

quoted

-       char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
+       char real_usage[HID_SENSOR_USAGE_LENGTH];
        struct platform_device *custom_pdev;
        const char *dev_name;
        char *c;
 
-       /* copy real usage id */
-       memcpy(real_usage, known_sensor_luid[index], 4);
+       memcpy(real_usage, match->luid, 4);

I ommitted the line adding the null terminator to the string but
kept 
that I didn't initialize the 'real_usage' as { 0 } anymore. The
string 
now misses the null terminator which leads to the broken utf-8.

The simple fix is to reintroduce the 0 initialization in 
hid_sensor_register_platform_device. E.g.

-       char real_usage[HID_SENSOR_USAGE_LENGTH];
+       char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };

I didn't realize that the issue was a buffer overrun. I tested the
kernel built with this simple fix and it works ok now. i.e. this
patch is is all that's needed:

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;

quoted

Where do I need to submit a patch for this? And on which tree
should I 
base the patch?

The change is so small it shouldn't require any rebasing. Just send
the
patch to these emails (from MAINTAINERS):

HID SENSOR HUB DRIVERS
M:  Jiri Kosina [off-list ref]
M:  Jonathan Cameron [off-list ref]
M:  Srinivas Pandruvada [off-list ref]
L:  linux-input@vger.kernel.org
L:  linux-iio@vger.kernel.org

quoted

I'm sorry for the problems my patch caused.

No problem. It actually made sleepgraph better because it exposed a
bug
in the ftrace processing code. I wasn't properly handling the corner
case where ftrace had binary data in it.

quoted

Regards,
Philipp Jungkamp

On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:

quoted

+Even

On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:

quoted

Hi all, 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 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

Here tag is:
.tag = "INT",
.luid = "020B000000000000",


The LUID is still a string. Probably too long for a dev_name.

Even,

Please check.

Thanks.
Srinivas

quoted

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. Can you rewrite this to ensure that
no
binary
chars make it into the devname?