Linux ARM-MSM sub-architecture
 help / color / mirror / Atom feed
* [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()
@ 2026-04-14 17:32 Youssef Samir
  2026-04-15 16:52 ` Lizhi Hou
  2026-05-12 16:30 ` Jeff Hugo
  0 siblings, 2 replies; 5+ messages in thread
From: Youssef Samir @ 2026-04-14 17:32 UTC (permalink / raw)
  To: jeff.hugo, carl.vanderlip, troy.hanson, zachary.mckevitt
  Cc: ogabbay, lizhi.hou, karol.wachowski, linux-arm-msm, dri-devel,
	Ruikai Peng

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);
 		/* request must have timed out, drop packet */
 		trace_qaic_manage(NULL, "Packet dropped.", -ETIME);
 		kfree(msg);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()
  2026-04-14 17:32 [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker() Youssef Samir
@ 2026-04-15 16:52 ` Lizhi Hou
  2026-05-12 16:29   ` Jeff Hugo
  2026-05-12 16:30 ` Jeff Hugo
  1 sibling, 1 reply; 5+ messages in thread
From: Lizhi Hou @ 2026-04-15 16:52 UTC (permalink / raw)
  To: Youssef Samir, jeff.hugo, carl.vanderlip, troy.hanson,
	zachary.mckevitt
  Cc: ogabbay, karol.wachowski, linux-arm-msm, dri-devel, Ruikai Peng


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);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()
  2026-04-15 16:52 ` Lizhi Hou
@ 2026-05-12 16:29   ` Jeff Hugo
  2026-05-13 16:51     ` Lizhi Hou
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff Hugo @ 2026-05-12 16:29 UTC (permalink / raw)
  To: Lizhi Hou, Youssef Samir, carl.vanderlip, troy.hanson,
	zachary.mckevitt
  Cc: ogabbay, karol.wachowski, linux-arm-msm, dri-devel, Ruikai Peng

On 4/15/2026 10:52 AM, Lizhi Hou wrote:
> 
> 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.

In practice, there can only be one QAIC_TRANS_DEACTIVATE_FROM_DEV 
instance per message, so the two behaviors are functionally the same.

This is somewhat an argument for this change because having different 
behaviors across the code feels like a recipe for problems from corner 
cases.

-Jeff

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()
  2026-04-14 17:32 [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker() Youssef Samir
  2026-04-15 16:52 ` Lizhi Hou
@ 2026-05-12 16:30 ` Jeff Hugo
  1 sibling, 0 replies; 5+ messages in thread
From: Jeff Hugo @ 2026-05-12 16:30 UTC (permalink / raw)
  To: Youssef Samir, carl.vanderlip, troy.hanson, zachary.mckevitt
  Cc: ogabbay, lizhi.hou, karol.wachowski, linux-arm-msm, dri-devel,
	Ruikai Peng

On 4/14/2026 11:32 AM, 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>

Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker()
  2026-05-12 16:29   ` Jeff Hugo
@ 2026-05-13 16:51     ` Lizhi Hou
  0 siblings, 0 replies; 5+ messages in thread
From: Lizhi Hou @ 2026-05-13 16:51 UTC (permalink / raw)
  To: Jeff Hugo, Youssef Samir, carl.vanderlip, troy.hanson,
	zachary.mckevitt
  Cc: ogabbay, karol.wachowski, linux-arm-msm, dri-devel, Ruikai Peng


On 5/12/26 09:29, Jeff Hugo wrote:
> On 4/15/2026 10:52 AM, Lizhi Hou wrote:
>>
>> 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.
>
> In practice, there can only be one QAIC_TRANS_DEACTIVATE_FROM_DEV 
> instance per message, so the two behaviors are functionally the same.
>
> This is somewhat an argument for this change because having different 
> behaviors across the code feels like a recipe for problems from corner 
> cases.
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>
> -Jeff

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-13 16:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-14 17:32 [PATCH] accel/qaic: Address potential out-of-bounds read in resp_worker() Youssef Samir
2026-04-15 16:52 ` Lizhi Hou
2026-05-12 16:29   ` Jeff Hugo
2026-05-13 16:51     ` Lizhi Hou
2026-05-12 16:30 ` Jeff Hugo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox