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

[Daniel Wagner]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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 :)