netdev - Re: [Intel-wired-lan] [PATCH v2] igb: Fix string truncation warnings in igb_set_fw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Jan 2024 14:46:42 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Kunwu Chan <chentao@...inos.cn>
Cc: jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, Kunwu Chan <kunwu.chan@...mail.com>,
 przemyslaw.kitszel@...el.com, linux-kernel@...r.kernel.org,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 jacob.e.keller@...el.com
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Fix string truncation warnings
 in igb_set_fw_version

Dear Kunwu,


Thank you for your patch. I have some minor nits.

Am 12.01.24 um 03:58 schrieb Kunwu Chan:
> 'commit 1978d3ead82c ("intel: fix string truncation warnings")'

Please don’t enclose it in '': Commit 1978d3ead82c ("intel: fix string 
truncation warnings")

> fix '-Wformat-truncation=' warnings in igb_main.c by using kasprintf.

fix*es*

> kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.

Maybe paste one warning message.

> Fix this warning by using a larger space for adapter->fw_version,
> and then fall back and continue to use snprintf.
> 
> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
> Signed-off-by: Kunwu Chan <chentao@...inos.cn>
> Cc: Kunwu Chan <kunwu.chan@...mail.com>
> Suggested-by: Jakub Kicinski <kuba@...nel.org>


Kind regards,

Paul Menzel


> ---
> v2: Fall back to use snprintf and a larger space,as suggested by
> https://lore.kernel.org/all/20231212132637.1b0fb8aa@kernel.org/
> ---
>   drivers/net/ethernet/intel/igb/igb.h      |  2 +-
>   drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++-----------
>   2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index a2b759531cb7..3c2dc7bdebb5 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -637,7 +637,7 @@ struct igb_adapter {
>   		struct timespec64 period;
>   	} perout[IGB_N_PEROUT];
>   
> -	char fw_version[32];
> +	char fw_version[48];
>   #ifdef CONFIG_IGB_HWMON
>   	struct hwmon_buff *igb_hwmon_buff;
>   	bool ets;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b2295caa2f0a..ce762d77d2c1 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3069,7 +3069,6 @@ void igb_set_fw_version(struct igb_adapter *adapter)
>   {
>   	struct e1000_hw *hw = &adapter->hw;
>   	struct e1000_fw_version fw;
> -	char *lbuf;
>   
>   	igb_get_fw_version(hw, &fw);
>   
> @@ -3077,34 +3076,36 @@ void igb_set_fw_version(struct igb_adapter *adapter)
>   	case e1000_i210:
>   	case e1000_i211:
>   		if (!(igb_get_flash_presence_i210(hw))) {
> -			lbuf = kasprintf(GFP_KERNEL, "%2d.%2d-%d",
> -					 fw.invm_major, fw.invm_minor,
> -					 fw.invm_img_type);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%2d.%2d-%d",
> +				 fw.invm_major, fw.invm_minor,
> +				 fw.invm_img_type);
>   			break;
>   		}
>   		fallthrough;
>   	default:
>   		/* if option rom is valid, display its version too */
>   		if (fw.or_valid) {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x, %d.%d.%d",
> -					 fw.eep_major, fw.eep_minor,
> -					 fw.etrack_id, fw.or_major, fw.or_build,
> -					 fw.or_patch);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d, 0x%08x, %d.%d.%d",
> +				 fw.eep_major, fw.eep_minor, fw.etrack_id,
> +				 fw.or_major, fw.or_build, fw.or_patch);
>   		/* no option rom */
>   		} else if (fw.etrack_id != 0X0000) {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x",
> -					 fw.eep_major, fw.eep_minor,
> -					 fw.etrack_id);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d, 0x%08x",
> +				 fw.eep_major, fw.eep_minor, fw.etrack_id);
>   		} else {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d.%d", fw.eep_major,
> -					 fw.eep_minor, fw.eep_build);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d.%d",
> +				 fw.eep_major, fw.eep_minor, fw.eep_build);
>   		}
>   		break;
>   	}
> -
> -	/* the truncate happens here if it doesn't fit */
> -	strscpy(adapter->fw_version, lbuf, sizeof(adapter->fw_version));
> -	kfree(lbuf);
>   }
>   
>   /**