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 <[email protected]>
> ---
> 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 <[email protected]>
--
Kees Cook