linux-kernel - Re: [PATCH V1] accel/amdxdna: Fix incorrect size of ERT_START

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2beea319-0a2d-8e29-0a57-6b43a85a00f4@amd.com>
Date: Thu, 10 Apr 2025 09:11:27 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: "Falkowski, Maciej" <maciej.falkowski@...ux.intel.com>,
	<ogabbay@...nel.org>, <quic_jhugo@...cinc.com>,
	<dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
	<sonal.santan@....com>, <king.tam@....com>
Subject: Re: [PATCH V1] accel/amdxdna: Fix incorrect size of ERT_START_NPU
 commands


On 4/10/25 00:27, Falkowski, Maciej wrote:
> On 4/9/2025 11:00 PM, Lizhi Hou wrote:
>
>> When multiple ERT_START_NPU commands are combined in one buffer, the
>> buffer size calculation is incorrect. Also, the condition to make sure
>> the buffer size is not beyond 4K is also fixed.
>>
>> Fixes: aac243092b70 ("accel/amdxdna: Add command execution")
>> Signed-off-by: Lizhi Hou <lizhi.hou@....com>
>> ---
>>   drivers/accel/amdxdna/aie2_message.c  |  6 +++---
>>   drivers/accel/amdxdna/aie2_msg_priv.h | 10 ++++------
>>   2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_message.c 
>> b/drivers/accel/amdxdna/aie2_message.c
>> index bf4219e32cc1..82412eec9a4b 100644
>> --- a/drivers/accel/amdxdna/aie2_message.c
>> +++ b/drivers/accel/amdxdna/aie2_message.c
>> @@ -525,7 +525,7 @@ aie2_cmdlist_fill_one_slot_cf(void *cmd_buf, u32 
>> offset,
>>       if (!payload)
>>           return -EINVAL;
>>   -    if (!slot_cf_has_space(offset, payload_len))
>> +    if (!slot_has_space(*buf, offset, payload_len))
>>           return -ENOSPC;
>>         buf->cu_idx = cu_idx;
>> @@ -558,7 +558,7 @@ aie2_cmdlist_fill_one_slot_dpu(void *cmd_buf, u32 
>> offset,
>>       if (payload_len < sizeof(*sn) || arg_sz > MAX_DPU_ARGS_SIZE)
>>           return -EINVAL;
>>   -    if (!slot_dpu_has_space(offset, arg_sz))
>> +    if (!slot_has_space(*buf, offset, arg_sz))
>>           return -ENOSPC;
>>         buf->inst_buf_addr = sn->buffer;
>> @@ -569,7 +569,7 @@ aie2_cmdlist_fill_one_slot_dpu(void *cmd_buf, u32 
>> offset,
>>       memcpy(buf->args, sn->prop_args, arg_sz);
>>         /* Accurate buf size to hint firmware to do necessary copy */
>> -    *size += sizeof(*buf) + arg_sz;
>> +    *size = sizeof(*buf) + arg_sz;
>>       return 0;
>>   }
>>   diff --git a/drivers/accel/amdxdna/aie2_msg_priv.h 
>> b/drivers/accel/amdxdna/aie2_msg_priv.h
>> index 4e02e744b470..6df9065b13f6 100644
>> --- a/drivers/accel/amdxdna/aie2_msg_priv.h
>> +++ b/drivers/accel/amdxdna/aie2_msg_priv.h
>> @@ -319,18 +319,16 @@ struct async_event_msg_resp {
>>   } __packed;
>>     #define MAX_CHAIN_CMDBUF_SIZE SZ_4K
>> -#define slot_cf_has_space(offset, payload_size) \
>> -    (MAX_CHAIN_CMDBUF_SIZE - ((offset) + (payload_size)) > \
>> -     offsetof(struct cmd_chain_slot_execbuf_cf, args[0]))
>
> Could this macro be rewritten as static inline function?
> That would provide additional typecheck.

Thanks for your suggestion. slot_cf_has_space and slot_dpu_has_space are 
merged into one macro to reduce duplicate code.

The new macro has slot parameter which could be either cf slot or dpu 
slot type. Thus, it may not use inline function.


Lizhi

>
>> +#define slot_has_space(slot, offset, payload_size)        \
>> +    (MAX_CHAIN_CMDBUF_SIZE >= (offset) + (payload_size) + \
>> +     sizeof(typeof(slot)))
>> +
>>   struct cmd_chain_slot_execbuf_cf {
>>       __u32 cu_idx;
>>       __u32 arg_cnt;
>>       __u32 args[] __counted_by(arg_cnt);
>>   };
>>   -#define slot_dpu_has_space(offset, payload_size) \
>> -    (MAX_CHAIN_CMDBUF_SIZE - ((offset) + (payload_size)) > \
>> -     offsetof(struct cmd_chain_slot_dpu, args[0]))
>>   struct cmd_chain_slot_dpu {
>>       __u64 inst_buf_addr;
>>       __u32 inst_size;
>
> Reviewed-by: Maciej Falkowski <maciej.falkowski@...ux.intel.com>
>
> Best regards,
> Maciej