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

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

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