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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4141c9ab-c640-4765-a23c-c2f64df687cb@stanley.mountain>
Date: Tue, 7 Jan 2025 15:30:05 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Karan Tilak Kumar <kartilak@...co.com>
Cc: sebaddel@...co.com, arulponn@...co.com, djhawar@...co.com,
	gcboffa@...co.com, mkai2@...co.com, satishkh@...co.com,
	aeasi@...co.com, jejb@...ux.ibm.com, martin.petersen@...cle.com,
	linux-scsi@...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 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