Re: [PATCH 2/2] scsi: ufs: Serve the device init reads from one aggregated read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential unaligned memory access when parsing malicious or buggy UFS 5.0 aggregated read packets.
--

commit 6958e42c99c17e67c3aba30be65b988c84328c42
Author: Hyeoncheol Jeong <hyenc.jeong@samsung.com>

scsi: ufs: Serve the device init reads from one aggregated read

The commit caches a UFS 5.0 AGGREGATED READ payload into a buffer during
initialization and serves individual attribute, flag, and string descriptor
requests from the cached packet.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e0254ddb31729..05a2f4532be5d 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -3816,6 +3816,313 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
>  	return err;
>  }
>  
[ ... ]
> +/**
> + * ufshcd_agg_group - find a group in the cached aggregated data packet
> + * @hba: per-adapter instance
> + * @type: group type to find
> + * @group_len: set to the group payload length on a hit
> + *
> + * Return: the group payload, or NULL if @type is not present.
> + */
> +static const u8 *ufshcd_agg_group(const struct ufs_hba *hba, u8 type,
> +				  u16 *group_len)
> +{
> +	const u8 *packet = hba->agg_packet;
> +	u16 hdr_off = 0;
> +
> +	while (packet && hdr_off + QUERY_AGG_GROUP_HDR_SIZE <= hba->agg_packet_len) {
> +		const struct utp_agg_group_header *hdr =
> +			(const void *)(packet + hdr_off);
> +		u16 next_off = be16_to_cpu(hdr->next_group_offset);

[Severity: High]
Can this cause an unaligned memory access crash on strict-alignment
architectures?

Since struct utp_agg_group_header lacks the __packed attribute, the compiler
expects instances of this struct to be 2-byte aligned.

If a UFS device returns an odd value for next_group_offset in the payload,
hdr_off will be odd on the next iteration. Casting packet + hdr_off to
struct utp_agg_group_header * and dereferencing hdr->next_group_offset
would cause the compiler to emit an aligned 16-bit load instruction.

If the pointer is unaligned, this can lead to a hardware alignment exception
when dereferenced. Should this code validate the alignment, use
get_unaligned_be16(), or define struct utp_agg_group_header as __packed?

> +		u16 payload_off = hdr_off + QUERY_AGG_GROUP_HDR_SIZE;
> +
> +		if (next_off && (next_off < payload_off ||
> +				 next_off + QUERY_AGG_GROUP_HDR_SIZE > hba->agg_packet_len))
> +			return NULL;
> +
> +		if (hdr->group_type == type) {
> +			*group_len = (next_off ? next_off : hba->agg_packet_len) -
> +				     payload_off;
> +			return packet + payload_off;
> +		}
> +
> +		if (!next_off)
> +			break;
> +
> +		hdr_off = next_off;
> +	}
> +
> +	return NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914083838epcms2p1336cbd08ce6ed192eeed9adbad365a93@epcms2p1?part=2