On 9/8/25 13:13, Dan Carpenter wrote:
On Mon, Sep 08, 2025 at 11:19:33AM -0700, Lizhi Hou wrote:
On 9/7/25 23:40, Dan Carpenter wrote:
Hello Lizhi Hou,
Commit 2f509fe6a42c ("accel/amdxdna: Add ioctl
DRM_IOCTL_AMDXDNA_GET_ARRAY") from Sep 2, 2025 (linux-next), leads to
the following (UNPUBLISHED) Smatch static checker warning:
drivers/accel/amdxdna/aie2_pci.c:904 aie2_query_ctx_status_array()
warn: potential user controlled sizeof overflow
'args->num_element * args->element_size' '1-u32max(user) *
1-u32max(user)'
drivers/accel/amdxdna/aie2_pci.c
891 static int aie2_query_ctx_status_array(struct amdxdna_client *client,
892 struct amdxdna_drm_get_array
*args)
893 {
894 struct amdxdna_drm_get_array array_args;
895 struct amdxdna_dev *xdna = client->xdna;
896 struct amdxdna_client *tmp_client;
897 int ret;
898
899 drm_WARN_ON(&xdna->ddev, !mutex_is_locked(&xdna->dev_lock));
900
901 array_args.element_size = min(args->element_size,
902 sizeof(struct
amdxdna_drm_hwctx_entry));
Instead of min() here we should just return -EINVAL if they are !=.
The request element_size from runtime tools can be smaller or bigger than
sizeof(struct amdxdna_drm_hwctx_entry).
If element_size is smaller, element_size bytes will be copied to user space.
If it is bigger, sizeof(struct amdxdna_drm_hwctx_entry) bytes will be
copied.
And the actual element size and number of element will be returned to
userspace.
This is a new API. We should be strict about what kind of inputs we
allow so that if we want to in the future we can change it without
breaking things.
Supplying a larger value is not useful at all. We should return -EINVAL
for that.
I'm guessing userspace already takes advantage of passing a smaller
value but it's not documented why this is required. My guess is that
maybe at times at times we just want the ->context_id or something?
Maybe the first three members of the struct?
This ioctl is to show hwctx information by user space tools. It happens
multiple times that more information needs to be shown by adding new
field to the end of this structure. So the protocol is as below.
Assumptions:
sizeof structure v1 < sizeof structure v2
Tool v1 and driver v1 are compiled with structure v1
Tool v2 and driver v2 are compiled with structure v2
1) Tool v2 can run with driver v1 (request element_size > structure
size), the new added data field in structure v2 will not be shown in
this case. The tool v2 would not force user to upgrade driver in this case.
2) Tool v1 can run with driver v2 (request element_size < structure
size).
Thanks,
Lizhi
903 array_args.buffer = args->buffer;
--> 904 array_args.num_element = args->num_element * args->element_size
/
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
These are both u32 values controlled by the user so this is an integer
overflow bug. Security bug.
This will not cause an issue. array_args.num_element is considered as user
control as well.
That's true.
If it is too big, the actual number of active hwctx will be returned.
It is better to put a reasonable limitation. I would add a check
(args->num_element < 1K && args->element_size < 4K). Will this fix the
smatch warning?
Yes. Anything which prevents an integer oveflow is sufficient. Thanks!
regards,
dan carpenter
