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 [email protected]
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 [email protected]
Reviewed-by: Jonathan Cameron [email protected]
Acked-by: Srinivas Pandruvada [email protected]
Signed-off-by: Jiri Kosina [email protected]
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
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?
+Even
On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> 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 [email protected]
> 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 [email protected]
> Reviewed-by: Jonathan Cameron [email protected]
> Acked-by: Srinivas Pandruvada [email protected]
> Signed-off-by: Jiri Kosina [email protected]
>
> 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
> 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?
>
Hello,
on v3 of the patchset I had this comment on the 'real_usage'
initialization:
> > - 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:
> - 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 };
Where do I need to submit a patch for this? And on which tree should I
base the patch?
I'm sorry for the problems my patch caused.
Regards,
Philipp Jungkamp
On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> +Even
>
> On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > 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 [email protected]
> > 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 [email protected]
> > Reviewed-by: Jonathan Cameron [email protected]
> > Acked-by: Srinivas Pandruvada
> > [email protected]
> > Signed-off-by: Jiri Kosina [email protected]
> >
> > 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
>
>
> > 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?
> >
>
On Fri, 2023-03-10 at 15:35 +0100, Philipp Jungkamp wrote:
> Hello,
>
> on v3 of the patchset I had this comment on the 'real_usage'
> initialization:
>
> > > - 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:
>
> > - 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;
> 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 <[email protected]>
M: Jonathan Cameron <[email protected]>
M: Srinivas Pandruvada <[email protected]>
L: [email protected]
L: [email protected]
> 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.
> Regards,
> Philipp Jungkamp
>
> On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > +Even
> >
> > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > 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 [email protected]
> > > 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 [email protected]
> > > Reviewed-by: Jonathan Cameron [email protected]
> > > Acked-by: Srinivas Pandruvada
> > > [email protected]
> > > Signed-off-by: Jiri Kosina [email protected]
> > >
> > > 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
> >
> >
> > > 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?
> > >
>
>
Hi, All,
Sorry for response later.
From below description, it seems not a buffer overrun issue, it's just caused by NULL terminated string, right?
Best Regards,
Even Xu
-----Original Message-----
From: Todd Brandt <[email protected]>
Sent: Saturday, March 11, 2023 7:36 AM
To: Philipp Jungkamp <[email protected]>; srinivas pandruvada <[email protected]>; [email protected]; [email protected]; Xu, Even <[email protected]>
Cc: [email protected]; [email protected]; Brandt, Todd E <[email protected]>
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:
> Hello,
>
> on v3 of the patchset I had this comment on the 'real_usage'
> initialization:
>
> > > - 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:
>
> > - 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;
> 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 <[email protected]>
M: Jonathan Cameron <[email protected]>
M: Srinivas Pandruvada <[email protected]>
L: [email protected]
L: [email protected]
> 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.
> Regards,
> Philipp Jungkamp
>
> On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > +Even
> >
> > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > 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 [email protected]
> > > 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 [email protected]
> > > Reviewed-by: Jonathan Cameron [email protected]
> > > Acked-by: Srinivas Pandruvada
> > > [email protected]
> > > Signed-off-by: Jiri Kosina [email protected]
> > >
> > > 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
> >
> >
> > > 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?
> > >
>
>
On Fri, 2023-03-17 at 05:49 +0000, Xu, Even wrote:
> Hi, All,
>
> Sorry for response later.
>
> From below description, it seems not a buffer overrun issue, it's
> just caused by NULL terminated string, right?
>
Correct, the subject may be a bit misleading, it's just a for loop
reading past the end of a string because of the lack of a NULL char.
The patch adds the NULL char.
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Todd Brandt <[email protected]>
> Sent: Saturday, March 11, 2023 7:36 AM
> To: Philipp Jungkamp <[email protected]>; srinivas pandruvada
> <[email protected]>; [email protected];
> [email protected]; Xu, Even <[email protected]>
> Cc: [email protected]; [email protected]; Brandt, Todd E
> <[email protected]>
> 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:
> > Hello,
> >
> > on v3 of the patchset I had this comment on the 'real_usage'
> > initialization:
> >
> > > > - 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:
> >
> > > - 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;
>
> > 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 <[email protected]>
> M: Jonathan Cameron <[email protected]>
> M: Srinivas Pandruvada <[email protected]>
> L: [email protected]
> L: [email protected]
>
> > 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.
>
> > Regards,
> > Philipp Jungkamp
> >
> > On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > > +Even
> > >
> > > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > > 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 [email protected]
> > > > 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 [email protected]
> > > > Reviewed-by: Jonathan Cameron [email protected]
> > > > Acked-by: Srinivas Pandruvada
> > > > [email protected]
> > > > Signed-off-by: Jiri Kosina [email protected]
> > > >
> > > > 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
> > >
> > >
> > > > 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?
> > > >
> >
> >
>
Got it, Thanks Todd!
Best Regards,
Even Xu
-----Original Message-----
From: Todd Brandt <[email protected]>
Sent: Friday, March 17, 2023 11:38 PM
To: Xu, Even <[email protected]>; Philipp Jungkamp <[email protected]>; srinivas pandruvada <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; Brandt, Todd E <[email protected]>
Subject: Re: BUG: hid-sensor-ids code includes binary data in device name
On Fri, 2023-03-17 at 05:49 +0000, Xu, Even wrote:
> Hi, All,
>
> Sorry for response later.
>
> From below description, it seems not a buffer overrun issue, it's just
> caused by NULL terminated string, right?
>
Correct, the subject may be a bit misleading, it's just a for loop reading past the end of a string because of the lack of a NULL char.
The patch adds the NULL char.
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Todd Brandt <[email protected]>
> Sent: Saturday, March 11, 2023 7:36 AM
> To: Philipp Jungkamp <[email protected]>; srinivas pandruvada
> <[email protected]>; [email protected];
> [email protected]; Xu, Even <[email protected]>
> Cc: [email protected]; [email protected]; Brandt, Todd E
> <[email protected]>
> 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:
> > Hello,
> >
> > on v3 of the patchset I had this comment on the 'real_usage'
> > initialization:
> >
> > > > - 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:
> >
> > > - 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;
>
> > 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 <[email protected]>
> M: Jonathan Cameron <[email protected]>
> M: Srinivas Pandruvada <[email protected]>
> L: [email protected]
> L: [email protected]
>
> > 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.
>
> > Regards,
> > Philipp Jungkamp
> >
> > On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > > +Even
> > >
> > > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > > 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 [email protected]
> > > > 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 [email protected]
> > > > Reviewed-by: Jonathan Cameron [email protected]
> > > > Acked-by: Srinivas Pandruvada
> > > > [email protected]
> > > > Signed-off-by: Jiri Kosina [email protected]
> > > >
> > > > 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
> > >
> > >
> > > > 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?
> > > >
> >
> >
>