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 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?
> >
>
