linux-kernel - Re: [PATCH v2] scsi: ufs: Zero utp_upiu_req at the beginning of each command

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <83fed524-a235-493c-81f6-16736027eeb1@acm.org>
Date: Mon, 16 Sep 2024 15:20:07 -0700
From: Bart Van Assche <bvanassche@....org>
To: Avri Altman <avri.altman@....com>,
 "Martin K . Petersen" <martin.petersen@...cle.com>
Cc: linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] scsi: ufs: Zero utp_upiu_req at the beginning of each
 command

On 9/15/24 12:48 AM, Avri Altman wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8ea5a82503a9..1f6575afc1c5 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2761,7 +2761,6 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
>   	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
>   
>   	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
> -	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
>   	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
>   
>   	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
> @@ -2834,6 +2833,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
>   	u8 upiu_flags;
>   	int ret = 0;
>   
> +	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> +
>   	ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0);
>   
>   	if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY)
> @@ -2858,6 +2859,8 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>   	unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
>   	u8 upiu_flags;
>   
> +	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> +
>   	ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, lrbp->cmd->sc_data_direction, 0);
>   	if (ioprio_class == IOPRIO_CLASS_RT)
>   		upiu_flags |= UPIU_CMD_FLAGS_CP;
> @@ -7165,6 +7168,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>   
>   	ufshcd_setup_dev_cmd(hba, lrbp, cmd_type, 0, tag);
>   
> +	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> +
>   	ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0);
>   
>   	/* update the task tag in the request upiu */
> @@ -7317,6 +7322,8 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
>   
>   	ufshcd_setup_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, UFS_UPIU_RPMB_WLUN, tag);
>   
> +	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> +
>   	ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, ehs);
>   
>   	/* update the task tag */

Something unfortunate about the above patch is that it spreads the
initialization of *lrbp->ucd_req_ptr over two functions. Wouldn't it be
better to have all the code that initializes *lrbp->ucd_req_ptr in the
same function, e.g. as in the untested patch below?

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8d90af6434da..9d826e5d610b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2770,13 +2770,14 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct 
ufshcd_lrb *lrbp, u8 upiu_flags)
  		.task_tag = lrbp->task_tag,
  		.command_set_type = UPIU_COMMAND_SET_TYPE_SCSI,
  	};
+	memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
+	       sizeof(ucd_req_ptr->header));

  	WARN_ON_ONCE(ucd_req_ptr->header.task_tag != lrbp->task_tag);

  	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);

  	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
-	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
  	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);

  	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
@@ -2809,6 +2810,8 @@ static void 
ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
  				cpu_to_be16(len) :
  				0,
  	};
+	memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
+	       sizeof(ucd_req_ptr->header));

  	/* Copy the Query Request buffer as is */
  	memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
@@ -2825,12 +2828,12 @@ static inline void 
ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
  {
  	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;

-	memset(ucd_req_ptr, 0, sizeof(struct utp_upiu_req));
-
  	ucd_req_ptr->header = (struct utp_upiu_header){
  		.transaction_code = UPIU_TRANSACTION_NOP_OUT,
  		.task_tag = lrbp->task_tag,
  	};
+	memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
+	       sizeof(ucd_req_ptr->header));

  	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
  }

Thanks,

Bart.