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] [day] [month] [year] [list]
Date: Mon, 15 Jan 2024 14:38:10 +0800
From: Kunwu Chan <chentao@...inos.cn>
To: Paul Menzel <pmenzel@...gen.mpg.de>
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

Hi Paul,
Thanks for your reply.

On 2024/1/12 21:46, Paul Menzel wrote:
> 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*

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

Thanks again.
> 
>> 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);
>>   }
>>   /**
-- 
Thanks,
   Kunwu