RE: [PATCH v15 3/3] Input: new da7280 haptic driver
On Fri, July 3, 2020 3:02 AM, Jes Sorensen wrote:
> On 6/29/20 9:01 AM, Roy Im wrote:
> > Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with multiple
> > mode and integrated waveform memory and wideband support.
> > It communicates via an I2C bus to the device.
> >
> > Signed-off-by: Roy Im
> > ---
> > v15:
> > - Removed some defines and updated some comments.
> > v14:
> > - Updated pwm related code, alignments and comments.
> > v13:
> > - Updated some conditions in pwm function and alignments.
> > v12: No changes.
> > v11:
> > - Updated the pwm related code, comments and typo.
> > v10:
> > - Updated the pwm related function and added some comments.
> > v9:
> > - Removed the header file and put the definitions into the c file.
> > - Updated the pwm code and error logs with %pE
> > v8:
> > - Added changes to support FF_PERIODIC/FF_CUSTOM and FF_CONSTANT.
> > - Updated the dt-related code.
> > - Removed memless related functions.
> > v7:
> > - Added more attributes to handle one value per file.
> > - Replaced and updated the dt-related code and functions called.
> > - Fixed error/functions.
> > v6: No changes.
> > v5: Fixed errors in Kconfig file.
> > v4: Updated code as dt-bindings are changed.
> > v3: No changes.
> > v2: Fixed kbuild error/warning
> >
> >
> > drivers/input/misc/Kconfig | 13 +
> > drivers/input/misc/Makefile |1 +
> > drivers/input/misc/da7280.c | 1838
> > +++
> > 3 files changed, 1852 insertions(+)
> > create mode 100644 drivers/input/misc/da7280.c
>
> [snip]
>
> > +static ssize_t
> > +patterns_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf,
> > + size_t count)
> > +{
> > + struct da7280_haptic *haptics = dev_get_drvdata(dev);
> > + char cmd[MAX_USER_INPUT_LEN];
> > + struct parse_data_t mem;
> > + unsigned int val;
> > + int error;
> > +
> > + error = regmap_read(haptics->regmap, DA7280_MEM_CTL1, );
> > + if (error)
> > + return error;
> > +
> > + if (count > MAX_USER_INPUT_LEN)
> > + memcpy(cmd, buf, MAX_USER_INPUT_LEN);
> > + else
> > + memcpy(cmd, buf, count);
> > +
> > + /* chop of '\n' introduced by echo at the end of the input */
> > + if (cmd[count - 1] == '\n')
> > + cmd[count - 1] = '\0';
>
> You have a potential memory corruption bug here for the case where count >
> MAX_USER_INPUT_LEN. The code
> correctly clamps the memcpy() length, but it still is at risk of writing
> beyond the end of the cmd buffer when doing the \0
> termination.
>
> If you change the code above to say
>
> if (count > MAX_USER_INPUT_LEN)
> count = MAX_USER_INPUT_LEN
> memcpy(cmd, buf, count);
>
> it should take care of it, and it will also return the actual count written
> to the caller.
You are right and thanks for your comment, I will change the code as you
suggested.
Kind regards,
Roy
Re: [PATCH v15 3/3] Input: new da7280 haptic driver
On 6/29/20 9:01 AM, Roy Im wrote:
> Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with
> multiple mode and integrated waveform memory and wideband support.
> It communicates via an I2C bus to the device.
>
> Signed-off-by: Roy Im
> ---
> v15:
> - Removed some defines and updated some comments.
> v14:
> - Updated pwm related code, alignments and comments.
> v13:
> - Updated some conditions in pwm function and alignments.
> v12: No changes.
> v11:
> - Updated the pwm related code, comments and typo.
> v10:
> - Updated the pwm related function and added some comments.
> v9:
> - Removed the header file and put the definitions into the c file.
> - Updated the pwm code and error logs with %pE
> v8:
> - Added changes to support FF_PERIODIC/FF_CUSTOM and FF_CONSTANT.
> - Updated the dt-related code.
> - Removed memless related functions.
> v7:
> - Added more attributes to handle one value per file.
> - Replaced and updated the dt-related code and functions called.
> - Fixed error/functions.
> v6: No changes.
> v5: Fixed errors in Kconfig file.
> v4: Updated code as dt-bindings are changed.
> v3: No changes.
> v2: Fixed kbuild error/warning
>
>
> drivers/input/misc/Kconfig | 13 +
> drivers/input/misc/Makefile |1 +
> drivers/input/misc/da7280.c | 1838
> +++
> 3 files changed, 1852 insertions(+)
> create mode 100644 drivers/input/misc/da7280.c
[snip]
> +static ssize_t
> +patterns_store(struct device *dev,
> +struct device_attribute *attr,
> +const char *buf,
> +size_t count)
> +{
> + struct da7280_haptic *haptics = dev_get_drvdata(dev);
> + char cmd[MAX_USER_INPUT_LEN];
> + struct parse_data_t mem;
> + unsigned int val;
> + int error;
> +
> + error = regmap_read(haptics->regmap, DA7280_MEM_CTL1, );
> + if (error)
> + return error;
> +
> + if (count > MAX_USER_INPUT_LEN)
> + memcpy(cmd, buf, MAX_USER_INPUT_LEN);
> + else
> + memcpy(cmd, buf, count);
> +
> + /* chop of '\n' introduced by echo at the end of the input */
> + if (cmd[count - 1] == '\n')
> + cmd[count - 1] = '\0';
You have a potential memory corruption bug here for the case where
count > MAX_USER_INPUT_LEN. The code correctly clamps the memcpy()
length, but it still is at risk of writing beyond the end of the cmd
buffer when doing the \0 termination.
If you change the code above to say
if (count > MAX_USER_INPUT_LEN)
count = MAX_USER_INPUT_LEN
memcpy(cmd, buf, count);
it should take care of it, and it will also return the actual count
written to the caller.
Cheers,
Jes
RE: [PATCH v15 3/3] Input: new da7280 haptic driver
On Tue, June 30, 2020 12:14 PM, Randy Dunlap wrote: > On 6/29/20 6:01 AM, Roy Im wrote: > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > > index 362e8a0..79fbddb 100644 > > --- a/drivers/input/misc/Kconfig > > +++ b/drivers/input/misc/Kconfig > > @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY > > To compile this driver as a module, choose M here: the > > module will be called stpmic1_onkey. > > > > +config INPUT_DA7280_HAPTICS > > + tristate "Dialog Semiconductor DA7280 haptics support" > > + depends on INPUT && I2C > > + select INPUT_FF_MEMLESS > > + select REGMAP_I2C > > + help > > + Say Y to enable support for the Dialog DA7280 haptics driver. > > + The haptics can be controlled by i2c communication, > > If you make any more updates, make this: I2C communication, > please. OK, I will do. Thanks for your comment. > > > + or by PWM input, or by GPI. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called da7280. > > + > > endif > > thanks. > -- > ~Randy
Re: [PATCH v15 3/3] Input: new da7280 haptic driver
Hi Roy, On 6/29/20 6:01 AM, Roy Im wrote: > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > index 362e8a0..79fbddb 100644 > --- a/drivers/input/misc/Kconfig > +++ b/drivers/input/misc/Kconfig > @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY > To compile this driver as a module, choose M here: the > module will be called stpmic1_onkey. > > +config INPUT_DA7280_HAPTICS > + tristate "Dialog Semiconductor DA7280 haptics support" > + depends on INPUT && I2C > + select INPUT_FF_MEMLESS > + select REGMAP_I2C > + help > + Say Y to enable support for the Dialog DA7280 haptics driver. > + The haptics can be controlled by i2c communication, If you make any more updates, make this: I2C communication, please. > + or by PWM input, or by GPI. > + > + To compile this driver as a module, choose M here: the > + module will be called da7280. > + > endif thanks. -- ~Randy
