Re: [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 3/2/20 12:33 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> > Make goodix_send_cfg() take a raw buffer as argument instead of a
> > struct firmware *cfg, so that it can also be used to restore the
> > config
> > on resume if necessary.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> > BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> > Cc: Dmitry Mastykin <mastichi@xxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >   drivers/input/touchscreen/goodix.c | 46 ++++++++++++++------------
> > ----
> >   1 file changed, 21 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c
> > b/drivers/input/touchscreen/goodix.c
> > index 21be33384d14..0f39c499e3a9 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -71,7 +71,7 @@ enum goodix_irq_pin_access_method {
> >   struct goodix_chip_data {
> >   	u16 config_addr;
> >   	int config_len;
> > -	int (*check_config)(struct goodix_ts_data *, const struct
> > firmware *);
> > +	int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg,
> > int len);
>
> Any way to make the length a uint instead of an int? That way, we
> wouldn't need to add < 0 guards, and the "len > MAX_LENGTH" check would
> be enough.

Actually the code does things like:

	int raw_cfg_len = len - 3;

        for (i = 0; i < raw_cfg_len; i += 2)
                check_sum += get_unaligned_be16(&ts->config[i]);
	check_sum = (~check_sum) + 1;
	if (check_sum != get_unaligned_be16(&cfg[raw_cfg_len])) {

Which is bad without a minimum check regardless of us using signed
or unsigned ints here. unsigned ints will short-circuit / skip the for
loop when len < 3, but then we end up with a negative array index when
doing: &cfg[raw_cfg_len]. And when going unsigned then both the loop
and the array index will be out of bounds when len < 3. So the proper
fix here is to add a minimum check.

I will add an extra patch to the patch-set adding a minimum bounds check.

Regards,

Hans

	


> Looks good otherwise:
>
> Reviewed-by: Bastien Nocera <hadess@xxxxxxxxxx>
>
> >   	void (*fix_config)(struct goodix_ts_data *ts);
> >   };
> >   
> > @@ -101,9 +101,9 @@ struct goodix_ts_data {
> >   };
> >   
> >   static int goodix_check_cfg_8(struct goodix_ts_data *ts,
> > -			const struct firmware *cfg);
> > +			      const u8 *cfg, int len);
> >   static int goodix_check_cfg_16(struct goodix_ts_data *ts,
> > -			const struct firmware *cfg);
> > +			       const u8 *cfg, int len);
> >   static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
> >   static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
> >   
> > @@ -426,22 +426,21 @@ static int goodix_request_irq(struct
> > goodix_ts_data *ts)
> >   					 ts->irq_flags, ts->client-
> > > name, ts);
> >   }
> >   
> > -static int goodix_check_cfg_8(struct goodix_ts_data *ts,
> > -			const struct firmware *cfg)
> > +static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8
> > *cfg, int len)
> >   {
> > -	int i, raw_cfg_len = cfg->size - 2;
> > +	int i, raw_cfg_len = len - 2;
> >   	u8 check_sum = 0;
> >   
> >   	for (i = 0; i < raw_cfg_len; i++)
> > -		check_sum += cfg->data[i];
> > +		check_sum += cfg[i];
> >   	check_sum = (~check_sum) + 1;
> > -	if (check_sum != cfg->data[raw_cfg_len]) {
> > +	if (check_sum != cfg[raw_cfg_len]) {
> >   		dev_err(&ts->client->dev,
> >   			"The checksum of the config fw is not
> > correct");
> >   		return -EINVAL;
> >   	}
> >   
> > -	if (cfg->data[raw_cfg_len + 1] != 1) {
> > +	if (cfg[raw_cfg_len + 1] != 1) {
> >   		dev_err(&ts->client->dev,
> >   			"Config fw must have Config_Fresh register
> > set");
> >   		return -EINVAL;
> > @@ -463,22 +462,22 @@ static void goodix_fix_cfg_8(struct
> > goodix_ts_data *ts)
> >   	ts->config[raw_cfg_len + 1] = 1;
> >   }
> >   
> > -static int goodix_check_cfg_16(struct goodix_ts_data *ts,
> > -			const struct firmware *cfg)
> > +static int goodix_check_cfg_16(struct goodix_ts_data *ts, const u8
> > *cfg,
> > +			       int len)
> >   {
> > -	int i, raw_cfg_len = cfg->size - 3;
> > +	int i, raw_cfg_len = len - 3;
> >   	u16 check_sum = 0;
> >   
> >   	for (i = 0; i < raw_cfg_len; i += 2)
> > -		check_sum += get_unaligned_be16(&cfg->data[i]);
> > +		check_sum += get_unaligned_be16(&cfg[i]);
> >   	check_sum = (~check_sum) + 1;
> > -	if (check_sum != get_unaligned_be16(&cfg->data[raw_cfg_len])) {
> > +	if (check_sum != get_unaligned_be16(&cfg[raw_cfg_len])) {
> >   		dev_err(&ts->client->dev,
> >   			"The checksum of the config fw is not
> > correct");
> >   		return -EINVAL;
> >   	}
> >   
> > -	if (cfg->data[raw_cfg_len + 2] != 1) {
> > +	if (cfg[raw_cfg_len + 2] != 1) {
> >   		dev_err(&ts->client->dev,
> >   			"Config fw must have Config_Fresh register
> > set");
> >   		return -EINVAL;
> > @@ -506,16 +505,15 @@ static void goodix_fix_cfg_16(struct
> > goodix_ts_data *ts)
> >    * @ts: goodix_ts_data pointer
> >    * @cfg: firmware config data
> >    */
> > -static int goodix_check_cfg(struct goodix_ts_data *ts,
> > -			    const struct firmware *cfg)
> > +static int goodix_check_cfg(struct goodix_ts_data *ts, const u8
> > *cfg, int len)
> >   {
> > -	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
> > +	if (len > GOODIX_CONFIG_MAX_LENGTH) {
> >   		dev_err(&ts->client->dev,
> >   			"The length of the config fw is not correct");
> >   		return -EINVAL;
> >   	}
> >   
> > -	return ts->chip->check_config(ts, cfg);
> > +	return ts->chip->check_config(ts, cfg, len);
> >   }
> >   
> >   /**
> > @@ -524,17 +522,15 @@ static int goodix_check_cfg(struct
> > goodix_ts_data *ts,
> >    * @ts: goodix_ts_data pointer
> >    * @cfg: config firmware to write to device
> >    */
> > -static int goodix_send_cfg(struct goodix_ts_data *ts,
> > -			   const struct firmware *cfg)
> > +static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg,
> > int len)
> >   {
> >   	int error;
> >   
> > -	error = goodix_check_cfg(ts, cfg);
> > +	error = goodix_check_cfg(ts, cfg, len);
> >   	if (error)
> >   		return error;
> >   
> > -	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
> > cfg->data,
> > -				 cfg->size);
> > +	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
> > cfg, len);
> >   	if (error) {
> >   		dev_err(&ts->client->dev, "Failed to write config data:
> > %d",
> >   			error);
> > @@ -1058,7 +1054,7 @@ static void goodix_config_cb(const struct
> > firmware *cfg, void *ctx)
> >   
> >   	if (cfg) {
> >   		/* send device configuration to the firmware */
> > -		error = goodix_send_cfg(ts, cfg);
> > +		error = goodix_send_cfg(ts, cfg->data, cfg->size);
> >   		if (error)
> >   			goto err_release_cfg;
> >   	}