lkml.org 
[lkml]   [2023]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: BUG: hid-sensor-ids code includes binary data in device name
    Date
    Got it, Thanks Todd!

    Best Regards,
    Even Xu

    -----Original Message-----
    From: Todd Brandt <todd.e.brandt@linux.intel.com>
    Sent: Friday, March 17, 2023 11:38 PM
    To: Xu, Even <even.xu@intel.com>; Philipp Jungkamp <p.jungkamp@gmx.net>; srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
    Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E <todd.e.brandt@intel.com>
    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@linux.intel.com>
    > Sent: Saturday, March 11, 2023 7:36 AM
    > To: Philipp Jungkamp <p.jungkamp@gmx.net>; srinivas pandruvada
    > <srinivas.pandruvada@linux.intel.com>; linux-input@vger.kernel.org;
    > linux-kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
    > Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E
    > <todd.e.brandt@intel.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@kernel.org>
    > M:  Jonathan Cameron <jic23@kernel.org>
    > M:  Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
    > L:  linux-input@vger.kernel.org
    > L:  linux-iio@vger.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@gmx.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@gmx.net
    > > > >     Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
    > > > >     Acked-by: Srinivas Pandruvada
    > > > > srinivas.pandruvada@linux.intel.com
    > > > >     Signed-off-by: Jiri Kosina jkosina@suse.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?
    > > > >
    > >
    > >
    >

    \
     
     \ /
      Last update: 2023-03-27 01:07    [W:3.227 / U:1.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site