Re: BUG: hid-sensor-ids code includes binary data in device name

[Philipp Jungkamp]

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