linux-kernel - RE: [PATCH v7 07/15] scsi: fnic: Add and integrate support for FDMI

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:
 <SJ0PR11MB5896ED0B5D190413AA380EE1C31F2@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date: Mon, 13 Jan 2025 20:28:42 +0000
From: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To: John Meneghini <jmeneghi@...hat.com>, Dan Carpenter
	<dan.carpenter@...aro.org>
CC: "Sesidhar Baddela (sebaddel)" <sebaddel@...co.com>, "Arulprabhu Ponnusamy
 (arulponn)" <arulponn@...co.com>, "Dhanraj Jhawar (djhawar)"
	<djhawar@...co.com>, "Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>, "Masa
 Kai (mkai2)" <mkai2@...co.com>, "Satish Kharat (satishkh)"
	<satishkh@...co.com>, "Arun Easi (aeasi)" <aeasi@...co.com>,
	"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>, "martin.petersen@...cle.com"
	<martin.petersen@...cle.com>, "linux-scsi@...r.kernel.org"
	<linux-scsi@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, kernel test robot <lkp@...el.com>
Subject: RE: [PATCH v7 07/15] scsi: fnic: Add and integrate support for FDMI

On Monday, January 13, 2025 9:35 AM, John Meneghini <jmeneghi@...hat.com> wrote:
>
> Just a note to say that these patches are important to Red Hat and we are actively engaged in back porting and testing these patches in to RHEL-9 and RHEL-10.
>
> I think these issues that Dan has pointed out are all issues which can be addressed in a follow up patch.
>
> /John
>
> On 1/7/25 07:30, Dan Carpenter wrote:
> > On Wed, Dec 11, 2024 at 06:03:04PM -0800, Karan Tilak Kumar wrote:
> >> @@ -612,6 +615,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>    unsigned long flags;
> >>    int hwq;
> >>    char *desc, *subsys_desc;
> >> +  int len;
> >
> > Do not introduce unnecessary levels of indirection.  Get rid of this len
> > variable.
> >
> >>
> >>    /*
> >>     * Allocate SCSI Host and set up association between host,
> >> @@ -646,9 +650,17 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>    fnic_stats_debugfs_init(fnic);
> >>
> >>    /* Find model name from PCIe subsys ID */
> >> -  if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0)
> >> +  if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) {
> >>            dev_info(&fnic->pdev->dev, "Model: %s\n", subsys_desc);
> >> -  else {
> >> +
> >> +          /* Update FDMI model */
> >
> > This comment adds no information.  Delete it.
> >
> >> +          fnic->subsys_desc_len = strlen(subsys_desc);
> >
> > Keep in mind that strlen() does not count the NUL-terminator.
> >
> >> +          len = ARRAY_SIZE(fnic->subsys_desc);
> >
> > Use sizeof() when you are talking about bytes or chars.  For snprintf() and
> > other string functions, it's always sizeof() and never ARRAY_SIZE().
> >
> >> +          if (fnic->subsys_desc_len > len)
> >> +                  fnic->subsys_desc_len = len;
> >> +          memcpy(fnic->subsys_desc, subsys_desc, fnic->subsys_desc_len);
> >
> > So this is an 0-14 character buffer.  If fnic->subsys_desc_len is set to 14,
> > then the string is not NUL terminated.  This is how the buffer is used in
> > fdls_fdmi_register_hba()
> >
> >     strscpy_pad(data, fnic->subsys_desc, FNIC_FDMI_MODEL_LEN);
> >     data[FNIC_FDMI_MODEL_LEN - 1] = 0;
> >
> > This suggests that fnic->subsys_desc is expected to be NUL-terminated.
> > However FNIC_FDMI_MODEL_LEN is 12.  So in that case the last 3 characters
> > are removed.  LOL.  It's harmless but so very annoying.
> >
> > Also strscpy_pad() will ensure that data[FNIC_FDMI_MODEL_LEN - 1] is set
> > to zero so that line could be deleted.
> >
> > regards,
> > dan carpenter
> >
>
>

Thanks for your inputs, John.

Regards,
Karan