Re: [PATCH v2] scsi: ufs: Add support for the aggregated read query opcode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The dedicated device management response buffer (ALIGNED_DEVMAN_RSP_SIZE) is exactly 4096 bytes, failing to account for the 32-byte Query Response UPIU header. This leads to an out-of-bounds read into the adjacent PRDT array during a maximum-sized aggregated read, leaking kernel physical memory addresses to userspace.
--

--- Patch [1]: [PATCH v2] scsi: ufs: Add support for the aggregated read query opcode ---
commit b0be9d1800d5bb45da1d07516b525d85f757798b
Author: Hyeoncheol Jeong <hyenc.jeong@samsung.com>

scsi: ufs: Add support for the aggregated read query opcode

This commit introduces the AGGREGATED READ query opcode (0x9), returning an
aggregated data packet in a single query request. It allocates a dedicated
device management descriptor with a 4 KiB response area for the reserved tag
to handle the larger responses.

> diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
> index 58b506eac6dc4..176fedd496afe 100644
> --- a/drivers/ufs/core/ufs_bsg.c
> +++ b/drivers/ufs/core/ufs_bsg.c
> @@ -14,14 +14,18 @@
>  #include "ufshcd-priv.h"
>  
>  static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
> -				       struct utp_upiu_query *qr)
> +				       struct utp_upiu_query *qr,
> +				       enum query_opcode desc_op)
>  {
>  	int desc_size = be16_to_cpu(qr->length);
>  
>  	if (desc_size <= 0)
>  		return -EINVAL;
>  
> -	*desc_len = min_t(int, QUERY_DESC_MAX_SIZE, desc_size);
> +	if (desc_op == UPIU_QUERY_OPCODE_AGGREGATED_READ)
> +		*desc_len = min_t(int, QUERY_AGGREGATED_MAX_SIZE, desc_size);
> +	else
> +		*desc_len = min_t(int, QUERY_DESC_MAX_SIZE, desc_size);
>  
>  	return 0;
>  }

[Severity: High]
Is QUERY_AGGREGATED_MAX_SIZE allowing userspace to request a buffer that is
too large?

If userspace requests a 4096-byte desc_size (QUERY_AGGREGATED_MAX_SIZE), it
makes the subsequent response read up to 4096 bytes of payload data, which
may not fit into the dedicated devman response array once the 32-byte
UPIU header is included.

[ ... ]

> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index 9f0fdd850e54e..c4d563420152d 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -18,6 +18,8 @@ enum {
>  	TASK_REQ_UPIU_SIZE_DWORDS	= 8,
>  	TASK_RSP_UPIU_SIZE_DWORDS	= 8,
>  	ALIGNED_UPIU_SIZE		= 512,
> +	/* Larger response area, only for the devman UCD */
> +	ALIGNED_DEVMAN_RSP_SIZE		= 4096,
>  };
>  
>  /* UFSHCI Registers */

[ ... ]

> @@ -501,6 +503,15 @@ struct utp_transfer_cmd_desc {
>  	u8 prd_table[];
>  };
>  
> +/* Dedicated UCD for the devman/reserved slot */
> +struct utp_devman_cmd_desc {
> +	u8 command_upiu[ALIGNED_UPIU_SIZE];
> +	u8 response_upiu[ALIGNED_DEVMAN_RSP_SIZE];
> +	u8 prd_table[];
> +};
> +
> +static_assert(QUERY_AGGREGATED_MAX_SIZE <= ALIGNED_DEVMAN_RSP_SIZE);

[Severity: High]
Does ALIGNED_DEVMAN_RSP_SIZE need to be larger to safely accommodate the
UPIU header?

If the response data segment from the device is up to 4096 bytes
(QUERY_AGGREGATED_MAX_SIZE), and it follows the 32-byte header, the total
size needed here would be 4096 + 32 = 4128 bytes.

Since response_upiu is exactly 4096 bytes, wouldn't a maximum-sized aggregated
read overflow past the array and into the adjacent prd_table?

[ ... ]

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 79331c2bd38de..b30c314c75e6c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -7637,7 +7686,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  
>  	/* just copy the upiu response as it is */
>  	memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, sizeof(*rsp_upiu));
> -	if (desc_buff && desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
> +	if (desc_buff && (desc_op == UPIU_QUERY_OPCODE_READ_DESC ||
> +			  desc_op == UPIU_QUERY_OPCODE_AGGREGATED_READ)) {
>  		u8 *descp = (u8 *)lrbp->ucd_rsp_ptr + sizeof(*rsp_upiu);
>  		u16 resp_len = be16_to_cpu(lrbp->ucd_rsp_ptr->header
>  					   .data_segment_length);

[Severity: High]
Can the subsequent memcpy read past the end of the response_upiu array?

If a userspace process issues a BSG query for AGGREGATED_READ and asks for
up to 4096 bytes, and the device returns a data_segment_length of 4096,
the descp pointer here is offset by sizeof(*rsp_upiu) (32 bytes).

The code shortly after this in ufshcd_issue_devman_upiu_cmd() does:

    if (*buff_len >= resp_len) {
        memcpy(desc_buff, descp, resp_len);
        ...

Calling memcpy(desc_buff, descp, 4096) will read 4096 bytes starting from
offset 32. Does this read past the end of the 4096-byte response_upiu array
and leak physical DMA addresses from the adjacent prd_table into the BSG
reply payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722084819epcms2p49c27fce999e821385f7b5d7ea5a02868@epcms2p4?part=1