Re: [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()

[Lizhi Hou <lizhi.hou@amd.com>]

On 4/14/26 10:32, Youssef Samir wrote:
> Although 'commit 2feec5ae5df7 ("accel/qaic: Handle DBC deactivation if the
> owner went away")' fixes the scenario it was intended for by walking the
> message and only decoding QAIC_TRANS_DEACTIVATE_FROM_DEV, if present, it
> skipped over the bounds checking code that is included in decode_message().
> This could lead to issues such as reading past the slab allocation's end,
> infinite loops or kernel panics. For those issues to happen, a malformed
> wire message is needed to be sent from the device.
>
> Instead of duplicating the bounds checking code already present in
> decode_message(), use the function inside resp_worker().
>
> Reported-by: Ruikai Peng <ruikai@pwno.io>
> Fixes: 2feec5ae5df7 ("accel/qaic: Handle DBC deactivation if the owner went away")
> Signed-off-by: Youssef Samir <youssef.abdulrahman@oss.qualcomm.com>
> ---
>   drivers/accel/qaic/qaic_control.c | 48 ++++++++++++++++---------------
>   1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index b21e6b5b3a10..818a77adde2a 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -1075,11 +1075,13 @@ static int decode_status(struct qaic_device *qdev, void *trans, struct manage_ms
>   
>   static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>   			  struct wire_msg *msg, struct ioctl_resources *resources,
> -			  struct qaic_user *usr)
> +			  struct qaic_user *usr, bool orphaned_deactivate)
>   {
> +	u32 msg_hdr_count = le32_to_cpu(msg->hdr.count);
>   	u32 msg_hdr_len = le32_to_cpu(msg->hdr.len);
>   	struct wire_trans_hdr *trans_hdr;
>   	u32 msg_len = 0;
> +	int trans_type;
>   	int ret;
>   	int i;
>   
> @@ -1089,13 +1091,15 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>   		return -EINVAL;
>   	}
>   
> -	user_msg->len = 0;
> -	user_msg->count = le32_to_cpu(msg->hdr.count);
> +	if (user_msg) {
> +		user_msg->len = 0;
> +		user_msg->count = msg_hdr_count;
> +	}
>   
>   	trace_qaic_manage_dbg(qdev->qddev, "Number of transaction to decode is %llu.",
> -			      user_msg->count);
> +			      msg_hdr_count);
>   
> -	for (i = 0; i < user_msg->count; ++i) {
> +	for (i = 0; i < msg_hdr_count; ++i) {
>   		u32 hdr_len;
>   
>   		if (msg_len > msg_hdr_len - sizeof(*trans_hdr))
> @@ -1110,7 +1114,20 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>   		trace_qaic_manage_dbg(qdev->qddev, "Decoding transaction %llu.",
>   				      le32_to_cpu(trans_hdr->type));
>   
> -		switch (le32_to_cpu(trans_hdr->type)) {
> +		trans_type = le32_to_cpu(trans_hdr->type);
> +		/*
> +		 * orphaned_deactivate is the case where a deactivate response
> +		 * is received from the device after the user owning the DBC,
> +		 * and the message requesting deactivation, has gone away.
> +		 * In this case, only process QAIC_TRANS_DEACTIVATE_FROM_DEV
> +		 * transaction and skip the others.
> +		 */
> +		if (orphaned_deactivate && trans_type != QAIC_TRANS_DEACTIVATE_FROM_DEV) {
> +			msg_len += hdr_len;
> +			continue;
> +		}
> +
> +		switch (trans_type) {
>   		case QAIC_TRANS_PASSTHROUGH_FROM_DEV:
>   			ret = decode_passthrough(qdev, trans_hdr, user_msg, &msg_len);
>   			break;
> @@ -1430,7 +1447,7 @@ static int qaic_manage(struct qaic_device *qdev, struct qaic_user *usr, struct m
>   		goto dma_cont_failed;
>   	}
>   
> -	ret = decode_message(qdev, user_msg, rsp, &resources, usr);
> +	ret = decode_message(qdev, user_msg, rsp, &resources, usr, false);
>   
>   dma_cont_failed:
>   	free_dbc_buf(qdev, &resources);
> @@ -1607,22 +1624,7 @@ static void resp_worker(struct work_struct *work)
>   		 * response to the QAIC_TRANS_TERMINATE_TO_DEV transaction,
>   		 * otherwise, the user can issue an soc_reset to the device.
>   		 */
> -		u32 msg_count = le32_to_cpu(msg->hdr.count);
> -		u32 msg_len = le32_to_cpu(msg->hdr.len);
> -		u32 len = 0;
> -		int j;
> -
> -		for (j = 0; j < msg_count && len < msg_len; ++j) {
> -			struct wire_trans_hdr *trans_hdr;
> -
> -			trans_hdr = (struct wire_trans_hdr *)(msg->data + len);
> -			if (le32_to_cpu(trans_hdr->type) == QAIC_TRANS_DEACTIVATE_FROM_DEV) {
> -				if (decode_deactivate(qdev, trans_hdr, &len, NULL))
> -					len += le32_to_cpu(trans_hdr->len);
> -			} else {
> -				len += le32_to_cpu(trans_hdr->len);
> -			}
> -		}
> +		decode_message(qdev, NULL, msg, NULL, NULL, true);

This seems changing the previous behavior. The original code will 
continue the loop when decode_deactivate() returns error. And 
decode_message() will error immediately when decode_deactivate() returns 
error.


Lizhi

>   		/* request must have timed out, drop packet */
>   		trace_qaic_manage(NULL, "Packet dropped.", -ETIME);
>   		kfree(msg);