* [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-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
* 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
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