Re: [PATCH 2/5] scsi: lpfc: Replace strlcat() with scnprintf() in lpfc_vport_symbolic_node_name()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing out-of-bounds read and stack buffer overflow in `lpfc_decode_firmware_rev` due to partial array initialization and missing null-termination.
--

--- Patch [2]: [PATCH 2/5] scsi: lpfc: Replace strlcat() with scnprintf() in lpfc_vport_symbolic_node_name() ---
commit ccac320efc7231c941f0ae4e81fa116ae23ad75e
Author: Ian Bridges <[email protected]>

scsi: lpfc: Replace strlcat() with scnprintf() in lpfc_vport_symbolic_node_name()

This commit replaces strlcat() API uses with a single scnprintf() in
lpfc_vport_symbolic_node_name(). The replacement formats all fragments
directly into the destination buffer, avoiding intermediate truncation.

Link: https://github.com/KSPP/linux/issues/370 [1]

> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
> index c7853e7fe0719..0734ab3be3e30 100644
> --- a/drivers/scsi/lpfc/lpfc_ct.c
> +++ b/drivers/scsi/lpfc/lpfc_ct.c
> @@ -1823,34 +1823,15 @@ lpfc_vport_symbolic_node_name(struct lpfc_vport *vport, char *symbol,
>  	size_t size)
>  {
>  	char fwrev[FW_REV_STR_SIZE] = {0};
> -	char tmp[MAXHOSTNAMELEN] = {0};
> -
> -	memset(symbol, 0, size);
> -
> -	scnprintf(tmp, sizeof(tmp), "Emulex %s", vport->phba->ModelName);
> -	if (strlcat(symbol, tmp, size) >= size)
> -		goto buffer_done;
>  
>  	lpfc_decode_firmware_rev(vport->phba, fwrev, 0);

[Severity: High]
This is a pre-existing issue, but can lpfc_decode_firmware_rev() cause a
stack buffer overflow when called from other paths with flag = 1?

Looking at lpfc_decode_firmware_rev() in drivers/scsi/lpfc/lpfc_ct.c, the
stack array str is only partially initialized:

drivers/scsi/lpfc/lpfc_ct.c:lpfc_decode_firmware_rev() {
        uint32_t *ptr, str[4];
        ...
                for (i = 0; i < 3; i++)
                        str[i] = be32_to_cpu(*ptr++);

                if (c == 0) {
                        if (flag)
                                sprintf(fwrevision, "%d.%d%d (%s)",
                                        b1, b2, b3, (char *)str);
                        else
...
}

Only 3 elements (12 bytes) of the 4-element str array are initialized.

If the firmware name does not contain any spaces to be converted to null
bytes in the first 12 bytes, won't sprintf() read past str[2] into
uninitialized stack memory?

Since this reads until a null byte is encountered, couldn't this overflow
the caller's fwrevision destination buffer?

This appears reachable when triggered via sysfs where untrusted device
firmware might provide maliciously crafted data.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2