Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

[Pavan Chebbi <pavan.chebbi@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4694 bytes --]

On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote:
> > On 7/5/24 13:26, Simon Horman wrote:
> > > Given the sizes of the buffers involved, it is theoretically
> > > possible for fw_ver_str to be truncated. Detect this and
> > > stop ethtool initialisation if this occurs.
> > >
> > > Flagged by gcc-14:
> > >
> > >    .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
> > >    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
> > >     4144 |                          "/pkg %s", buf);
> > >          |                                ^~   ~~~
> >
> > gcc is right, and you are right that we don't want such warnings
> > but I believe that the current flow is fine (copy as much as possible,
> > then proceed)
> >
> > >    In function 'bnxt_get_pkgver',
> > >        inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
> > >    .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
> > >     4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> > >          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >     4144 |                          "/pkg %s", buf);
> > >          |                          ~~~~~~~~~~~~~~~
> > >
> > > Compile tested only.
> > >
> > > Signed-off-by: Simon Horman <horms@kernel.org>
> > > ---
> > > It appears to me that size is underestimated by 1 byte -
> > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> > > because the size argument to snprintf should include the space for the
> > > trailing '\0'. But I have not changed that as it is separate from
> > > the issue this patch addresses.
> >
> > you are addressing "bad size" for copying strings around, I will just
> > fix that part too
>
> Right, I was thinking of handling that separately.
>
> > > ---
> > >   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
> > >   1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > index bf157f6cc042..5ccc3cc4ba7d 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
> > >     return rc;
> > >   }
> > > -static void bnxt_get_pkgver(struct net_device *dev)
> > > +static int bnxt_get_pkgver(struct net_device *dev)
> > >   {
> > >     struct bnxt *bp = netdev_priv(dev);
> > >     char buf[FW_VER_STR_LEN];
> > > -   int len;
> > >     if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
> > > -           len = strlen(bp->fw_ver_str);
> > > -           snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> > > -                    "/pkg %s", buf);
> > > +           int offset, size, rc;
> > > +
> > > +           offset = strlen(bp->fw_ver_str);
> > > +           size = FW_VER_STR_LEN - offset - 1;
> > > +
> > > +           rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
> > > +           if (rc >= size)
> > > +                   return -E2BIG;
> >
> > On error I would just replace last few bytes with "(...)" or "...", or
> > even "~". Other option is to enlarge bp->fw_ver_str, but I have not
> > looked there.
> >
> > >     }
> > > +
> > > +   return 0;
> > >   }
> > >   static int bnxt_get_eeprom(struct net_device *dev,
> > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
> > >     struct net_device *dev = bp->dev;
> > >     int i, rc;
> > > -   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> > > -           bnxt_get_pkgver(dev);
> > > +   if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> > > +           rc = bnxt_get_pkgver(dev);
> > > +           if (rc)
> > > +                   return;
> >
> > and here you are changing the flow, I would like to still init the
> > rest of the bnxt' ethtool stuff despite one informative string
> > being turncated
>
> Thanks, I'm fine with your suggestion.
> But I'll wait to see if there is feedback from others, especially Broadcom.

Hi Simon, thanks for the patch. I'd agree with Przemek. Would
definitely like to have the init complete just as before.

>
> > > +   }
> > >     bp->num_tests = 0;
> > >     if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
> > >
> >
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]