Re: [PATCH] input: iqs5xx: Fix incorrect argument passed to hex2bin

[Jeff LaBundy <jeff@labundy.com>]

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