From: Hans de Goede <hdegoede@redhat.com>
To: Bastien Nocera <hadess@hadess.net>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org, Dmitry Mastykin <mastichi@gmail.com>
Subject: Re: [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument
Date: Mon, 2 Mar 2020 19:53:35 +0100 [thread overview]
Message-ID: <0717ff90-3bc8-e08d-3b94-c700cd749bb0@redhat.com> (raw)
In-Reply-To: <55c1159deb1b7b43bb8b404ab9467eaae9ebbb85.camel@hadess.net>
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@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> 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@hadess.net>
>
>> 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;
>> }
>
next prev parent reply other threads:[~2020-03-02 18:53 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-21 16:47 [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses Hans de Goede
2020-02-21 16:47 ` [PATCH resend 02/10] Input: goodix - Make loading the config from disk independent from the GPIO setup Hans de Goede
2020-03-02 11:12 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 03/10] Input: goodix - Make resetting the controller at probe " Hans de Goede
2020-03-02 11:14 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices Hans de Goede
2020-03-02 11:23 ` Bastien Nocera
2020-03-02 15:40 ` Hans de Goede
2020-03-02 15:44 ` Bastien Nocera
2020-03-02 16:23 ` Hans de Goede
2020-02-21 16:47 ` [PATCH resend 05/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Bay " Hans de Goede
2020-03-02 11:24 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 06/10] Input: goodix - Add support for controlling the IRQ pin through ACPI methods Hans de Goede
2020-03-02 11:25 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 07/10] Input: goodix - Move defines to above struct goodix_ts_data declaration Hans de Goede
2020-03-02 11:25 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 08/10] Input: goodix - Save a copy of the config from goodix_read_config() Hans de Goede
2020-03-02 11:30 ` Bastien Nocera
2020-03-02 18:36 ` Hans de Goede
2020-02-21 16:47 ` [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument Hans de Goede
2020-03-02 11:33 ` Bastien Nocera
2020-03-02 18:53 ` Hans de Goede [this message]
2020-02-21 16:47 ` [PATCH resend 10/10] Input: goodix - Restore config on resume if necessary Hans de Goede
2020-03-02 11:35 ` Bastien Nocera
2020-03-02 19:08 ` Hans de Goede
2020-03-02 11:09 ` [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses Bastien Nocera
2020-03-02 13:23 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0717ff90-3bc8-e08d-3b94-c700cd749bb0@redhat.com \
--to=hdegoede@redhat.com \
--cc=dmitry.torokhov@gmail.com \
--cc=hadess@hadess.net \
--cc=linux-input@vger.kernel.org \
--cc=mastichi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).