| Message ID | 20250409-fix-lpfc-bios-str-v1-1-05dac9e51e13@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | lpfc: use memcpy for bios version | expand |
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!
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,
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 :)
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, "
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(-) --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20250409-fix-lpfc-bios-str-330f6a9d892f Best regards,