netdev - Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver

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]

Message-ID: <CACKFLikea+jP7y_82q5C6a+GUp5Ea8TCHUdjNoJdBUo=NTVYqQ@mail.gmail.com>
Date: Fri, 5 Jul 2024 10:39:14 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>
Cc: Simon Horman <horms@...nel.org>, Przemek Kitszel <przemyslaw.kitszel@...el.com>, 
	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

On Fri, Jul 5, 2024 at 10:03 AM Pavan Chebbi <pavan.chebbi@...adcom.com> wrote:
>
> On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@...nel.org> wrote:
> >
> > On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote:
> > > On 7/5/24 13:26, Simon Horman wrote:
> > > > 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.

Yes, please fix the size as well.

> > > >   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.
>

I agree as well.  We should continue with the rest of
bnxt_ethtool_init().  Thanks for the patch.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)