On 06.07.26 14:59, Christian Borntraeger wrote:
>
>
> Am 06.07.26 um 14:24 schrieb Dragos Tatulea:
>>
>>
>> On 06.07.26 12:13, Christian Borntraeger wrote:
>>>
>>>
>>> Am 06.07.26 um 12:01 schrieb Christian Borntraeger:
>>>> We have seen in our CI the following KASAN message:
>>>> BUG: KASAN: slab-out-of-bounds in cmd_exec+0x550/0xca0 [mlx5_core]
>>>> Read of size 272 at addr 0000000176795020 by task qemu-system-s39/82764
>>>> [...]
>>>> [<000011388ab3a7a0>] cmd_exec+0x550/0xca0 [mlx5_core]
>>>> [<000011388ab3b61c>] mlx5_cmd_exec_cb+0x25c/0x4f0 [mlx5_core]
>>>> [<000011388b21e82e>] mlx5_vdpa_exec_async_cmds+0x22e/0x5e0 [mlx5_vdpa]
>>>> [<000011388b21fd44>] create_direct_keys+0x954/0xef0 [mlx5_vdpa]
>>>> [...]
>>>> The buggy address is located 4128 bytes inside of
>>>> allocated 4384-byte region [0000000176794000, 0000000176795120)
>>>>
>>>> So in essence we read 16 bytes beyond 4384-byte allocation.
>>>> create_direct_keys calculates the pointer and length for in and out
>>>> buffers.
>>>> The size calculation for in includes the entire structure
>>>> size (out + in + mtt[]) but the pointer passed to cmd_exec points only
>>>> to the 'in' field, skipping the 'out' field.
>>>>
>>>> This causes mlx5_copy_to_msg() to read beyond the allocated buffer
>>>> by sizeof(out) bytes when copying command data.
>>>>
>>>> Calculates the input size as sizeof(in) + mtt_array_size to match the
>>>> pointer and allocation size.
>>>>
>>>> Fixes: 0071b138d44a ("vdpa/mlx5: Create direct MKEYs in parallel")
>>>> Signed-off-by: Christian Borntraeger <[email protected]>
>>>> ---
>>>> drivers/vdpa/mlx5/core/mr.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
>>>> index 6d02ccf9eb91..03fe7f5ca412 100644
>>>> --- a/drivers/vdpa/mlx5/core/mr.c
>>>> +++ b/drivers/vdpa/mlx5/core/mr.c
>>>> @@ -233,7 +233,7 @@ static int create_direct_keys(struct mlx5_vdpa_dev
>>>> *mvdev, struct mlx5_vdpa_mr *
>>>> cmds[i].out = cmd_mem->out;
>>>> cmds[i].outlen = sizeof(cmd_mem->out);
>>>> cmds[i].in = cmd_mem->in;
>>>> - cmds[i].inlen = struct_size(cmd_mem, mtt, mttcount);
>>>> + cmds[i].inlen = sizeof(cmd_mem->in) + (mttcount *
>>>> sizeof(cmd_mem->mtt[0]));
>> Woops... Thanks for the catch. The fix is good.
>>
>>>
>>> Assuming the fix is the correct fix, question is, if I should use the
>>> offset of in instead, e.g.
>>>
>>> cmds[i].inlen = struct_size(cmd_mem, mtt, mttcount) -
>>> offsetof(cmd_mem, in);
>>>
>>> to handle potential alignment and padding aspects.
>>>
>> Seems harder to read. Why not:
>> sizeof(cmd_mem->in) + flex_array_size(cmd_mem, mtts, mttcount) ?
>
> Can do that. I assume we do not have any implicit holes in the structure due
> to alignment? In
> other words, sizeof(cmd_mem->in) can never be something like 20 (and then the
> flex array would
> start at 24 to align 8 byte elements). If thats the case I will respin with
> your proposal.
Weeelll, I checked on a x86_64 build and there is a hole there of 16 bytes. So
your second
formula is indeed the way to go.
Thanks,
Dragos
