accel/amdxdna: Add ioctl DRM_IOCTL_AMDXDNA_GET_ARRAY
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 !=.
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.
905 array_args.element_size;
906 list_for_each_entry(tmp_client, &xdna->client_list, node) {
907 ret = amdxdna_hwctx_walk(tmp_client, &array_args,
908 aie2_hwctx_status_cb);
909 if (ret)
910 break;
911 }
912
913 args->element_size = array_args.element_size;
914 args->num_element = (u32)((array_args.buffer - args->buffer) /
915 args->element_size);
916
917 return ret;
918 }regards,
dan carpenter
Quoted 24 lines · Dan Carpenter
Hello Lizhi Hou,
Commit
2f509fe6a42c("accel/amdxdna: Add ioctlDRM_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.c891 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));
Quoted 1 line
Instead of
min()here we should just return-EINVALif 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.
Quoted 7 lines
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.
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?
Thanks,
Lizhi
Quoted 18 lines
905 array_args.element_size; 906 list_for_each_entry(tmp_client, &xdna->client_list, node) { 907 ret = amdxdna_hwctx_walk(tmp_client, &array_args, 908 aie2_hwctx_status_cb); 909 if (ret) 910 break; 911 } 912 913 args->element_size = array_args.element_size; 914 args->num_element = (u32)((array_args.buffer - args->buffer) / 915 args->element_size); 916 917 return ret; 918 }regards,
dan carpenter
Quoted 31 lines · Lizhi Hou
Quoted 24 lines · Dan Carpenter
Hello Lizhi Hou,
Commit
2f509fe6a42c("accel/amdxdna: Add ioctlDRM_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.c891 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));Quoted 1 line
Instead of
min()here we should just return-EINVALif they are !=.The request
element_sizefrom runtime tools can be smaller or bigger thansizeof(struct amdxdna_drm_hwctx_entry).
Quoted 8 lines
If
element_sizeis smaller,element_sizebytes 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?
Quoted 10 lines
Quoted 7 lines
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.
Quoted 7 lines
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
Quoted 42 lines · Dan Carpenter
Quoted 29 lines · Lizhi Hou
Quoted 24 lines · Dan Carpenter
Hello Lizhi Hou,
Commit
2f509fe6a42c("accel/amdxdna: Add ioctlDRM_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.c891 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));Quoted 1 line
Instead of
min()here we should just return-EINVALif they are !=.The request
element_sizefrom runtime tools can be smaller or bigger thansizeof(struct amdxdna_drm_hwctx_entry).Quoted 8 lines
If
element_sizeis smaller,element_sizebytes 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.
Quoted 7 lines
Supplying a larger value is not useful at all. We should return
-EINVALfor 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
Quoted 22 lines
Quoted 9 lines
Quoted 7 lines
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.
Quoted 6 lines
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