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

[Kunwu Chan]

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@xxxxxxxxxx>
> > Cc: Kunwu Chan <kunwu.chan@xxxxxxxxxxx>
> > Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx>
>
>
> Kind regards,
>
> Paul Menzel
>
>
> > ---
> > v2: Fall back to use snprintf and a larger space,as suggested by
> > https://lore.kernel.org/all/20231212132637.1b0fb8aa@xxxxxxxxxx/
> > ---
> >   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