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?
> >
> >
> > 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
