Message-ID: <DM6PR11MB2618B71FCDD70813404F570FF4BD9@DM6PR11MB2618.namprd11.prod.outlook.com>
Date:   Fri, 17 Mar 2023 05:49:34 +0000
From:   "Xu, Even" <even.xu@...el.com>
To:     "todd.e.brandt@...ux.intel.com" <todd.e.brandt@...ux.intel.com>,
        "Philipp Jungkamp" <p.jungkamp@....net>,
        srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "jkosina@...e.cz" <jkosina@...e.cz>,
        "Brandt, Todd E" <todd.e.brandt@...el.com>
Subject: RE: BUG: hid-sensor-ids code includes binary data in device name

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 <todd.e.brandt@...ux.intel.com> 
Sent: Saturday, March 11, 2023 7:36 AM
To: Philipp Jungkamp <p.jungkamp@....net>; srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>; linux-input@...r.kernel.org; linux-kernel@...r.kernel.org; Xu, Even <even.xu@...el.com>
Cc: Jonathan.Cameron@...wei.com; jkosina@...e.cz; Brandt, Todd E <todd.e.brandt@...el.com>
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@...nel.org>
M:  Jonathan Cameron <jic23@...nel.org>
M:  Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
L:  linux-input@...r.kernel.org
L:  linux-iio@...r.kernel.org

> 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@....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@....net
> > >     Reviewed-by: Jonathan Cameron Jonathan.Cameron@...wei.com
> > >     Acked-by: Srinivas Pandruvada
> > > srinivas.pandruvada@...ux.intel.com
> > >     Signed-off-by: Jiri Kosina jkosina@...e.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
> > 
> > 
> > > 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?
> > > 
> 
> 

Powered by blists - more mailing lists