lpfc: use memcpy for bios version

[PATCH] lpfc: use memcpy for bios version

Posted by Daniel Wagner 10 months ago

The strlcat with FORTIFY support is triggering a panic because it thinks
the target buffer will overflow although the correct target buffer
size is passed in.

Anyway, instead memset with 0 followed by a strlcat, just use memcpy and
ensure that the resulting buffer is NULL terminated.

BIOSVersion is only used for the lpfc_printf_log which expects a
properly terminated string.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/scsi/lpfc/lpfc_sli.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 6574f9e744766d49e245bd648667cc3ffc45289e..a335d34070d3c5fa4778bb1cb0eef797c7194f3b 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -6003,9 +6003,9 @@ lpfc_sli4_get_ctl_attr(struct lpfc_hba *phba)
 	phba->sli4_hba.flash_id = bf_get(lpfc_cntl_attr_flash_id, cntl_attr);
 	phba->sli4_hba.asic_rev = bf_get(lpfc_cntl_attr_asic_rev, cntl_attr);
 
-	memset(phba->BIOSVersion, 0, sizeof(phba->BIOSVersion));
-	strlcat(phba->BIOSVersion, (char *)cntl_attr->bios_ver_str,
+	memcpy(phba->BIOSVersion, cntl_attr->bios_ver_str,
 		sizeof(phba->BIOSVersion));
+	phba->BIOSVersion[sizeof(phba->BIOSVersion) - 1] = '\0';
 
 	lpfc_printf_log(phba, KERN_INFO, LOG_SLI,
 			"3086 lnk_type:%d, lnk_numb:%d, bios_ver:%s, "

---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250409-fix-lpfc-bios-str-330f6a9d892f

Best regards,
-- 
Daniel Wagner <wagi@kernel.org>

Re: [PATCH] lpfc: use memcpy for bios version

Posted by David Laight 9 months, 4 weeks ago

On Wed, 09 Apr 2025 13:34:22 +0200
Daniel Wagner <wagi@kernel.org> wrote:

> The strlcat with FORTIFY support is triggering a panic because it thinks
> the target buffer will overflow although the correct target buffer
> size is passed in.
> 
> Anyway, instead memset with 0 followed by a strlcat, just use memcpy and
> ensure that the resulting buffer is NULL terminated.
> 
> BIOSVersion is only used for the lpfc_printf_log which expects a
> properly terminated string.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>  drivers/scsi/lpfc/lpfc_sli.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 6574f9e744766d49e245bd648667cc3ffc45289e..a335d34070d3c5fa4778bb1cb0eef797c7194f3b 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -6003,9 +6003,9 @@ lpfc_sli4_get_ctl_attr(struct lpfc_hba *phba)
>  	phba->sli4_hba.flash_id = bf_get(lpfc_cntl_attr_flash_id, cntl_attr);
>  	phba->sli4_hba.asic_rev = bf_get(lpfc_cntl_attr_asic_rev, cntl_attr);
>  
> -	memset(phba->BIOSVersion, 0, sizeof(phba->BIOSVersion));
> -	strlcat(phba->BIOSVersion, (char *)cntl_attr->bios_ver_str,
> +	memcpy(phba->BIOSVersion, cntl_attr->bios_ver_str,
>  		sizeof(phba->BIOSVersion));
> +	phba->BIOSVersion[sizeof(phba->BIOSVersion) - 1] = '\0';

Isn't that just strscpy() ?

	David

>  
>  	lpfc_printf_log(phba, KERN_INFO, LOG_SLI,
>  			"3086 lnk_type:%d, lnk_numb:%d, bios_ver:%s, "
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250409-fix-lpfc-bios-str-330f6a9d892f
> 
> Best regards,

Re: [PATCH] lpfc: use memcpy for bios version

Posted by Daniel Wagner 9 months, 4 weeks ago

On Sun, Apr 13, 2025 at 07:02:38PM +0100, David Laight wrote:
> On Wed, 09 Apr 2025 13:34:22 +0200
> Daniel Wagner <wagi@kernel.org> wrote:
> 
> > The strlcat with FORTIFY support is triggering a panic because it thinks
> > the target buffer will overflow although the correct target buffer
> > size is passed in.

BTW, still trying to figure out what is happening here. It was observed
on ppc64el but so far creating a crash dump is not working.

> > Anyway, instead memset with 0 followed by a strlcat, just use memcpy and
> > ensure that the resulting buffer is NULL terminated.
> > 
> > BIOSVersion is only used for the lpfc_printf_log which expects a
> > properly terminated string.
> > 
> > Signed-off-by: Daniel Wagner <wagi@kernel.org>
> > ---
> >  drivers/scsi/lpfc/lpfc_sli.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> > index 6574f9e744766d49e245bd648667cc3ffc45289e..a335d34070d3c5fa4778bb1cb0eef797c7194f3b 100644
> > --- a/drivers/scsi/lpfc/lpfc_sli.c
> > +++ b/drivers/scsi/lpfc/lpfc_sli.c
> > @@ -6003,9 +6003,9 @@ lpfc_sli4_get_ctl_attr(struct lpfc_hba *phba)
> >  	phba->sli4_hba.flash_id = bf_get(lpfc_cntl_attr_flash_id, cntl_attr);
> >  	phba->sli4_hba.asic_rev = bf_get(lpfc_cntl_attr_asic_rev, cntl_attr);
> >  
> > -	memset(phba->BIOSVersion, 0, sizeof(phba->BIOSVersion));
> > -	strlcat(phba->BIOSVersion, (char *)cntl_attr->bios_ver_str,
> > +	memcpy(phba->BIOSVersion, cntl_attr->bios_ver_str,
> >  		sizeof(phba->BIOSVersion));
> > +	phba->BIOSVersion[sizeof(phba->BIOSVersion) - 1] = '\0';
> 
> Isn't that just strscpy() ?

strscpy does more work to ensure everything is correct and has the
advantage that it wont copy the whole buffer unnecessary. Given how
small the work is BIOSVersion is 8 bytes and bios_ver_str is 32 bytes
and there are other places in the driver doing something similar thing,
I opted for the traditional memcpy with an explicit NULLing. Obviously,
it also avoids using any of the fortify features :)

Re: [PATCH] lpfc: use memcpy for bios version

Posted by Daniel Wagner 9 months ago

On Mon, Apr 14, 2025 at 09:51:09AM +0200, Daniel Wagner wrote:
> On Sun, Apr 13, 2025 at 07:02:38PM +0100, David Laight wrote:
> > On Wed, 09 Apr 2025 13:34:22 +0200
> > Daniel Wagner <wagi@kernel.org> wrote:
> > 
> > > The strlcat with FORTIFY support is triggering a panic because it thinks
> > > the target buffer will overflow although the correct target buffer
> > > size is passed in.
> 
> BTW, still trying to figure out what is happening here. It was observed
> on ppc64el but so far creating a crash dump is not working.

FTR, finally got hold on the crash dump. As expected, the problem is
that bios_ver_str is not properly NUL terminated. strlcat expects that
both input strings are properly terminated.

Re: [PATCH] lpfc: use memcpy for bios version

Posted by Martin K. Petersen 10 months ago

Daniel,

> The strlcat with FORTIFY support is triggering a panic because it
> thinks the target buffer will overflow although the correct target
> buffer size is passed in.

Applied to 6.16/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH] lpfc: use memcpy for bios version

Posted by Justin Tee 10 months ago

Reviewed-by: Justin Tee <justin.tee@broadcom.com>

Thanks,
Justin Tee