linux-kernel - Re: [PATCH] lpfc: use memcpy for bios version

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] [day] [month] [year] [list]

Message-ID: <6f6b4f8c-e9f3-4962-af48-baf48a91c0a9@flourine.local>
Date: Mon, 14 Apr 2025 09:51:08 +0200
From: Daniel Wagner <dwagner@...e.de>
To: David Laight <david.laight.linux@...il.com>
Cc: Daniel Wagner <wagi@...nel.org>, 
	James Smart <james.smart@...adcom.com>, Dick Kennedy <dick.kennedy@...adcom.com>, 
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, "Martin K. Petersen" <martin.petersen@...cle.com>, 
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lpfc: use memcpy for bios version

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@...nel.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@...nel.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 :)