* Re: [PATCH] input: iqs5xx: Fix incorrect argument passed to hex2bin
2025-04-19 22:22 ` Jeff LaBundy
@ 2025-04-29 1:30 ` Dmitry Torokhov
0 siblings, 0 replies; 3+ messages in thread
From: Dmitry Torokhov @ 2025-04-29 1:30 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Purva Yeshi, Al Viro, linux-input, linux-kernel
On Sat, Apr 19, 2025 at 05:22:23PM -0500, Jeff LaBundy wrote:
> Hi Purva,
>
> On Sun, Apr 20, 2025 at 01:34:34AM +0530, Purva Yeshi wrote:
> > Fix Smatch-detected issue:
> > drivers/input/touchscreen/iqs5xx.c:747 iqs5xx_fw_file_parse()
> > error: hex2bin() 'rec->len' too small (2 vs 4)
> >
> > Fix incorrect second argument to hex2bin() when parsing firmware records.
> >
> > Pass a pointer to the ASCII hex data instead of the u8 record length to
> > hex2bin(), which expects a pointer, not an integer. The previous code
> > passed rec->len as the second argument, leading to undefined behavior
> > as hex2bin() attempted to read from an unintended memory address.
> >
> > Cast the entire rec structure to a const char * using a new pointer
> > rec_bytes. Skip the initial ':' character in the Intel HEX format by
> > passing rec_bytes + 1 to hex2bin(). This allows the function to decode
> > the 4-byte record header (length, address high, address low, and type)
> > correctly from its ASCII hex representation into binary form.
> >
> > Preserve the original code flow while ensuring correctness and resolving
> > the issue detected by Smatch.
> >
> > Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> > ---
> > drivers/input/touchscreen/iqs5xx.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
> > index 4ebd7565ae6e..e8140a54685f 100644
> > --- a/drivers/input/touchscreen/iqs5xx.c
> > +++ b/drivers/input/touchscreen/iqs5xx.c
> > @@ -744,7 +744,9 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
> > break;
> > }
> >
> > - error = hex2bin(rec_hdr, rec->len, sizeof(rec_hdr));
> > + const char *rec_bytes = (const char *)rec;
> > +
> > + error = hex2bin(rec_hdr, rec_bytes + 1, sizeof(rec_hdr));
> > +
> > if (error) {
> > dev_err(&client->dev, "Invalid header at record %u\n",
> > rec_num);
> > --
> > 2.34.1
> >
> >
>
> Thank you for the patch! I appreciate your having investigated this
> warning, but this patch is a NAK. I can't speak to why Smatch thinks
> there is a problem here, but we can see from the definition of the
> struct 'iqs5xx_ihex_rec' that 'len' is indeed a pointer:
>
> char len[2];
>
> I also checked with actual HW on latest kernel that FW updates still
> work just fine. The following line ensures we are looking at a valid
> memory location when locating the 'src' pointer:
>
> rec = (struct iqs5xx_ihex_rec *)(fw->data + pos);
>
> In case I have misunderstood, please let me know.
Right, I think the original submitter misread the code. However I find
the code a bit hard to follow with only some fields of iqs5xx_ihex_rec
being used directly and accessed spilling over to others.
I wonder if something like below will not make it easier to read...
Thanks.
--
Dmitry
diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 5b0c14ca2489..26aeb4a8ccde 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -72,7 +72,9 @@
#define IQS5XX_CSTM_LEN (IQS5XX_PMAP_END + 1 - IQS5XX_CSTM)
#define IQS5XX_PMAP_LEN (IQS5XX_PMAP_END + 1 - IQS5XX_CHKSM)
-#define IQS5XX_REC_HDR_LEN 4
+#define IQS5XX_REC_HDR_LEN_HEX (1 /* start */ + 2 /* size */ + \
+ 4 /* addr */ + 2 /* type */)
+#define IQS5XX_REC_HDR_LEN 4 /* size + addr (2 bytes) + type */
#define IQS5XX_REC_LEN_MAX 255
#define IQS5XX_REC_TYPE_DATA 0x00
#define IQS5XX_REC_TYPE_EOF 0x01
@@ -97,14 +99,6 @@ struct iqs5xx_dev_id_info {
u8 bl_status;
} __packed;
-struct iqs5xx_ihex_rec {
- char start;
- char len[2];
- char addr[4];
- char type[2];
- char data[2];
-} __packed;
-
struct iqs5xx_touch_data {
__be16 abs_x;
__be16 abs_y;
@@ -696,7 +690,6 @@ static irqreturn_t iqs5xx_irq(int irq, void *data)
static int iqs5xx_fw_file_parse(struct i2c_client *client,
const char *fw_file, u8 *pmap)
{
- struct iqs5xx_ihex_rec *rec;
size_t pos = 0;
int error, i;
u16 rec_num = 1;
@@ -723,25 +716,25 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
}
do {
- if (pos + sizeof(*rec) > fw->size) {
+ if (pos + IQS5XX_REC_HDR_LEN_HEX > fw->size) {
dev_err(&client->dev, "Insufficient firmware size\n");
return -EINVAL;
}
- rec = (struct iqs5xx_ihex_rec *)(fw->data + pos);
- pos += sizeof(*rec);
- if (rec->start != ':') {
+ if (fw->data[pos] != ':') {
dev_err(&client->dev, "Invalid start at record %u\n",
rec_num);
return -EINVAL;
}
- error = hex2bin(rec_hdr, rec->len, sizeof(rec_hdr));
+ /* Convert all 3 fields in one go */
+ error = hex2bin(rec_hdr, &fw->data[pos + 1], sizeof(rec_hdr));
if (error) {
dev_err(&client->dev, "Invalid header at record %u\n",
rec_num);
return error;
}
+ pos += IQS5XX_REC_HDR_LEN_HEX;
rec_len = *rec_hdr;
rec_addr = get_unaligned_be16(rec_hdr + sizeof(rec_len));
@@ -751,22 +744,22 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
dev_err(&client->dev, "Insufficient firmware size\n");
return -EINVAL;
}
- pos += (rec_len * 2);
- error = hex2bin(rec_data, rec->data, rec_len);
+ error = hex2bin(rec_data, &fw->data[pos], rec_len);
if (error) {
dev_err(&client->dev, "Invalid data at record %u\n",
rec_num);
return error;
}
+ pos += rec_len * 2;
- error = hex2bin(&rec_chksm,
- rec->data + rec_len * 2, sizeof(rec_chksm));
+ error = hex2bin(&rec_chksm, &fw->data[pos], sizeof(rec_chksm));
if (error) {
dev_err(&client->dev, "Invalid checksum at record %u\n",
rec_num);
return error;
}
+ pos += 2;
chksm = 0;
for (i = 0; i < sizeof(rec_hdr); i++)
^ permalink raw reply related [flat|nested] 3+ messages in thread