Received-SPF: pass (google.com: domain of linux-kernel+bounces-25661-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <50f1e711-12a2-432a-b571-c7b4f0e64ab1@kylinos.cn>
Date: Mon, 15 Jan 2024 14:38:10 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Fix string truncation warnings
 in igb_set_fw_version
Content-Language: en-US
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, Kunwu Chan <kunwu.chan@hotmail.com>,
 przemyslaw.kitszel@intel.com, linux-kernel@vger.kernel.org,
 intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
 jacob.e.keller@intel.com
References: <20240112025853.123048-1-chentao@kylinos.cn>
 <2a58fed3-1fa9-47eb-b475-3f7c3b291376@molgen.mpg.de>
From: Kunwu Chan <chentao@kylinos.cn>
In-Reply-To: <2a58fed3-1fa9-47eb-b475-3f7c3b291376@molgen.mpg.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable

Hi Paul,
Thanks for your reply.

On 2024/1/12 21:46, Paul Menzel wrote:
> Dear Kunwu,
>=20
>=20
> Thank you for your patch. I have some minor nits.
>=20
> Am 12.01.24 um 03:58 schrieb Kunwu Chan:
>> 'commit 1978d3ead82c ("intel: fix string truncation warnings")'
>=20
> Please don=E2=80=99t enclose it in '': Commit 1978d3ead82c ("intel: fix=
 string=20
> truncation warnings")
>=20
>> fix '-Wformat-truncation=3D' warnings in igb_main.c by using kasprintf=
.
>=20
> fix*es*

Thanks i'll rewirte the commit msg and add some warnings.
>=20
>> kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
>=20
> Maybe paste one warning message.

Thanks again.
>=20
>> 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@kylinos.cn>
>> Cc: Kunwu Chan <kunwu.chan@hotmail.com>
>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
>=20
>=20
> Kind regards,
>=20
> Paul Menzel
>=20
>=20
>> ---
>> v2: Fall back to use snprintf and a larger space,as suggested by
>> https://lore.kernel.org/all/20231212132637.1b0fb8aa@kernel.org/
>> ---
>> =C2=A0 drivers/net/ethernet/intel/igb/igb.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 |=C2=A0 2 +-
>> =C2=A0 drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++----=
-------
>> =C2=A0 2 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb.h=20
>> 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 {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct timespec=
64 period;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } perout[IGB_N_PEROUT];
>> -=C2=A0=C2=A0=C2=A0 char fw_version[32];
>> +=C2=A0=C2=A0=C2=A0 char fw_version[48];
>> =C2=A0 #ifdef CONFIG_IGB_HWMON
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct hwmon_buff *igb_hwmon_buff;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool ets;
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c=20
>> 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=20
>> *adapter)
>> =C2=A0 {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct e1000_hw *hw =3D &adapter->hw;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct e1000_fw_version fw;
>> -=C2=A0=C2=A0=C2=A0 char *lbuf;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 igb_get_fw_version(hw, &fw);
>> @@ -3077,34 +3076,36 @@ void igb_set_fw_version(struct igb_adapter=20
>> *adapter)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case e1000_i210:
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case e1000_i211:
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!(igb_get_f=
lash_presence_i210(hw))) {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lb=
uf =3D kasprintf(GFP_KERNEL, "%2d.%2d-%d",
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.invm_major, fw.inv=
m_minor,
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.invm_img_type);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sn=
printf(adapter->fw_version,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof(adapter->fw_version),
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%2d.%2d-%d",
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.invm_major, fw.invm_minor,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.invm_img_type);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 break;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fallthrough;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 default:
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* if option ro=
m is valid, display its version too */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (fw.or_valid=
) {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lb=
uf =3D kasprintf(GFP_KERNEL, "%d.%d, 0x%08x, %d.%d.%d",
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_major, fw.eep_=
minor,
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.etrack_id, fw.or_m=
ajor, fw.or_build,
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.or_patch);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sn=
printf(adapter->fw_version,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof(adapter->fw_version),
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%d.%d, 0x%08x, %d.%d.%d",
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_major, fw.eep_minor, fw.etrack_id,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.or_major, fw.or_build, fw.or_patch);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* no option ro=
m */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else if (fw.e=
track_id !=3D 0X0000) {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lb=
uf =3D kasprintf(GFP_KERNEL, "%d.%d, 0x%08x",
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_major, fw.eep_=
minor,
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.etrack_id);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sn=
printf(adapter->fw_version,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof(adapter->fw_version),
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%d.%d, 0x%08x",
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_major, fw.eep_minor, fw.etrack_id);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lb=
uf =3D kasprintf(GFP_KERNEL, "%d.%d.%d", fw.eep_major,
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_minor, fw.eep_=
build);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sn=
printf(adapter->fw_version,
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof(adapter->fw_version),
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%d.%d.%d",
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 fw.eep_major, fw.eep_minor, fw.eep_build);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>> -
>> -=C2=A0=C2=A0=C2=A0 /* the truncate happens here if it doesn't fit */
>> -=C2=A0=C2=A0=C2=A0 strscpy(adapter->fw_version, lbuf, sizeof(adapter-=
>fw_version));
>> -=C2=A0=C2=A0=C2=A0 kfree(lbuf);
>> =C2=A0 }
>> =C2=A0 /**
--=20
Thanks,
   Kunwu