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

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.

Kind regards,
Jeff LaBundy

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++)