| Message ID | 20240223-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v1-3-9cd3882f0700@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | scsi: replace deprecated strncpy | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Feb 23, 2024 at 10:23:08PM +0000, Justin Stitt wrote: > We expect slowpath_params.name to be NUL-terminated based on its future > usage with other string APIs: > > | static int qed_slowpath_start(struct qed_dev *cdev, > | struct qed_slowpath_params *params) > ... > | strscpy(drv_version.name, params->name, > | MCP_DRV_VER_STR_SIZE - 4); > > Moreover, NUL-padding is not necessary as the only use for this slowpath > name parameter is to copy into the drv_version.name field. > > Also, let's prefer using strscpy(src, dest, sizeof(src)) in two > instances (one of which is outside of the scsi system but it is trivial > and related to this patch). > > We can see the drv_version.name size here: > | struct qed_mcp_drv_version { > | u32 version; > | u8 name[MCP_DRV_VER_STR_SIZE - 4]; > | }; What a weird length. :P > > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > drivers/net/ethernet/qlogic/qed/qed_main.c | 2 +- > drivers/scsi/qedf/qedf_main.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c > index c278f8893042..d39e198fe8db 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_main.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c > @@ -1351,7 +1351,7 @@ static int qed_slowpath_start(struct qed_dev *cdev, > (params->drv_rev << 8) | > (params->drv_eng); > strscpy(drv_version.name, params->name, > - MCP_DRV_VER_STR_SIZE - 4); > + sizeof(drv_version.name)); > rc = qed_mcp_send_drv_version(hwfn, hwfn->p_main_ptt, > &drv_version); > if (rc) { > diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c > index a58353b7b4e8..fd12439cbaab 100644 > --- a/drivers/scsi/qedf/qedf_main.c > +++ b/drivers/scsi/qedf/qedf_main.c > @@ -3468,7 +3468,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode) > slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER; > slowpath_params.drv_rev = QEDF_DRIVER_REV_VER; > slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER; > - strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE); > + strscpy(slowpath_params.name, "qedf", sizeof(slowpath_params.name)); > rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params); > if (rc) { > QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n"); Yeah, looks good. Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c index c278f8893042..d39e198fe8db 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_main.c +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c @@ -1351,7 +1351,7 @@ static int qed_slowpath_start(struct qed_dev *cdev, (params->drv_rev << 8) | (params->drv_eng); strscpy(drv_version.name, params->name, - MCP_DRV_VER_STR_SIZE - 4); + sizeof(drv_version.name)); rc = qed_mcp_send_drv_version(hwfn, hwfn->p_main_ptt, &drv_version); if (rc) { diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c index a58353b7b4e8..fd12439cbaab 100644 --- a/drivers/scsi/qedf/qedf_main.c +++ b/drivers/scsi/qedf/qedf_main.c @@ -3468,7 +3468,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode) slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER; slowpath_params.drv_rev = QEDF_DRIVER_REV_VER; slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER; - strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE); + strscpy(slowpath_params.name, "qedf", sizeof(slowpath_params.name)); rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params); if (rc) { QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
We expect slowpath_params.name to be NUL-terminated based on its future usage with other string APIs: | static int qed_slowpath_start(struct qed_dev *cdev, | struct qed_slowpath_params *params) ... | strscpy(drv_version.name, params->name, | MCP_DRV_VER_STR_SIZE - 4); Moreover, NUL-padding is not necessary as the only use for this slowpath name parameter is to copy into the drv_version.name field. Also, let's prefer using strscpy(src, dest, sizeof(src)) in two instances (one of which is outside of the scsi system but it is trivial and related to this patch). We can see the drv_version.name size here: | struct qed_mcp_drv_version { | u32 version; | u8 name[MCP_DRV_VER_STR_SIZE - 4]; | }; Signed-off-by: Justin Stitt <justinstitt@google.com> --- drivers/net/ethernet/qlogic/qed/qed_main.c | 2 +- drivers/scsi/qedf/qedf_main.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)