Got it, Thanks Todd!
Best Regards,
Even Xu
-----Original Message-----
From: Todd Brandt <todd.e.brandt@xxxxxxxxxxxxxxx>
Sent: Friday, March 17, 2023 11:38 PM
To: Xu, Even <even.xu@xxxxxxxxx>; Philipp Jungkamp <p.jungkamp@xxxxxxx>; srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Cc: Jonathan.Cameron@xxxxxxxxxx; jkosina@xxxxxxx; Brandt, Todd E <todd.e.brandt@xxxxxxxxx>
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 <todd.e.brandt@xxxxxxxxxxxxxxx>
> Sent: Saturday, March 11, 2023 7:36 AM
> To: Philipp Jungkamp <p.jungkamp@xxxxxxx>; srinivas pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Xu, Even <even.xu@xxxxxxxxx>
> Cc: Jonathan.Cameron@xxxxxxxxxx; jkosina@xxxxxxx; Brandt, Todd E
> <todd.e.brandt@xxxxxxxxx>
> 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 <jikos@xxxxxxxxxx>
> M: Jonathan Cameron <jic23@xxxxxxxxxx>
> M: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> L: linux-input@xxxxxxxxxxxxxxx
> L: linux-iio@xxxxxxxxxxxxxxx
>
> > 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 p.jungkamp@xxxxxxx
> > > > 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@xxxxxxx
> > > > Reviewed-by: Jonathan Cameron Jonathan.Cameron@xxxxxxxxxx
> > > > Acked-by: Srinivas Pandruvada
> > > > srinivas.pandruvada@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Jiri Kosina jkosina@xxxxxxx
> > > >
> > > > 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?
> > > >
> >
> >
>
