Re: [PATCH V2] accel/amdxdna: Adjust size for copy_to_user()

[Lizhi Hou <lizhi.hou@amd.com>]

On 4/3/26 08:56, Mario Limonciello wrote:
> On 4/3/26 10:50 AM, Lizhi Hou wrote:
>>
>> On 4/2/26 14:06, Mario Limonciello wrote:
>>> On 4/2/26 12:41 PM, Lizhi Hou wrote:
>>>> The amount of data returned to user space should be limited by the 
>>>> buffer
>>>> size provided by the application. If the buffer is smaller than the 
>>>> data
>>>> size, return only the portion that fits instead of failing.
>>>>
>>>> Fixes: 850d71f6bf4c ("accel/amdxdna: Add query functions")
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>
> No more concerns after comments.
>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
Thanks, applied to drm-misc-next
>>>> ---
>>>>   drivers/accel/amdxdna/aie2_error.c   |  5 ++-
>>>>   drivers/accel/amdxdna/aie2_message.c | 20 ++++++----
>>>>   drivers/accel/amdxdna/aie2_pci.c     | 59 
>>>> ++++++++++++++++------------
>>>>   3 files changed, 50 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/accel/amdxdna/aie2_error.c b/drivers/accel/ 
>>>> amdxdna/aie2_error.c
>>>> index 9d20e956c020..70007b4363cd 100644
>>>> --- a/drivers/accel/amdxdna/aie2_error.c
>>>> +++ b/drivers/accel/amdxdna/aie2_error.c
>>>> @@ -406,8 +406,11 @@ int aie2_get_array_async_error(struct 
>>>> amdxdna_dev_hdl *ndev, struct amdxdna_drm_
>>>>         drm_WARN_ON(&xdna->ddev, !mutex_is_locked(&xdna->dev_lock));
>>>>   +    if (!args->num_element)
>>>> +        return -EINVAL;
>>>> +
>>>>       args->num_element = 1;
>>>
>>> I don't really follow.  Why check if args->num_element is 0 and then 
>>> set it to 1?  Does the check actually make sense? Just set it to one 
>>> and ignore what's there before.
>>
>> args->num_element is used as both user input and output argument.
>>
>> As a input argument, it means the max number of element user 
>> expected. Expecting 0 element doe not make sense, thus -EINVAL is 
>> returned.
>>
>> As a output argument, driver fills actual number of element being 
>> returned. One element is returned for getting async error case.
>>
>>>
>>> If it's for protecting args like a NULL args->buffer, wouldn't it 
>>> make more sense to check for that?
>>>
>>>> -    args->element_size = sizeof(ndev->last_async_err);
>>>> +    args->element_size = min(args->element_size, sizeof(ndev- 
>>>> >last_async_err));
>>>
>>> As this comes from userspace, couldn't it end up being clamped if 
>>> userspace sends a smaller buffer?  Is that intended?
>>
>> Yes, it is possible user sends smaller buffer. In this case, driver 
>> fills partial of its data. This is intended.
>>
>>
>>>
>>>>       if (copy_to_user(u64_to_user_ptr(args->buffer),
>>>>                &ndev->last_async_err, args->element_size))
>>>>           return -EFAULT;
>>>> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/ 
>>>> amdxdna/aie2_message.c
>>>> index e5e7da7a8f40..e52dc7ea9fc7 100644
>>>> --- a/drivers/accel/amdxdna/aie2_message.c
>>>> +++ b/drivers/accel/amdxdna/aie2_message.c
>>>> @@ -369,12 +369,13 @@ int aie2_query_status(struct amdxdna_dev_hdl 
>>>> *ndev, char __user *buf,
>>>>   {
>>>>       DECLARE_AIE_MSG(aie_column_info, MSG_OP_QUERY_COL_STATUS);
>>>>       struct amdxdna_dev *xdna = ndev->aie.xdna;
>>>> -    u32 buf_sz = size, aie_bitmap = 0;
>>>> +    u32 buf_sz, aie_bitmap = 0;
>>>>       struct amdxdna_client *client;
>>>>       dma_addr_t dma_addr;
>>>>       u8 *buff_addr;
>>>>       int ret;
>>>>   +    buf_sz = ndev->metadata.cols * ndev->metadata.size;
>>>
>>> Don't you want to respect /check the size of the caller immediately? 
>>> It seems odd to hardcode it here at allocation time.
>>
>> ndev->metadata.cols * ndev->metadata.size is from firmware but not 
>> user space. So it does not need to check.
>>
>>
>> Thanks,
>>
>> Lizhi
>>
>>>
>>>>       buff_addr = aie2_alloc_msg_buffer(ndev, &buf_sz, &dma_addr);
>>>>       if (IS_ERR(buff_addr))
>>>>           return PTR_ERR(buff_addr);
>>>> @@ -398,13 +399,14 @@ int aie2_query_status(struct amdxdna_dev_hdl 
>>>> *ndev, char __user *buf,
>>>>         XDNA_DBG(xdna, "Query NPU status completed");
>>>>   -    if (size < resp.size) {
>>>> +    if (buf_sz < resp.size) {
>>>>           ret = -EINVAL;
>>>> -        XDNA_ERR(xdna, "Bad buffer size. Available: %u. Needs: 
>>>> %u", size, resp.size);
>>>> +        XDNA_ERR(xdna, "Bad buffer size. Available: %u. Needs: 
>>>> %u", buf_sz, resp.size);
>>>>           goto fail;
>>>>       }
>>>>   -    if (copy_to_user(buf, buff_addr, resp.size)) {
>>>> +    size = min(size, resp.size);
>>>> +    if (copy_to_user(buf, buff_addr, size)) {
>>>>           ret = -EFAULT;
>>>>           XDNA_ERR(xdna, "Failed to copy NPU status to user space");
>>>>           goto fail;
>>>> @@ -424,13 +426,14 @@ int aie2_query_telemetry(struct 
>>>> amdxdna_dev_hdl *ndev,
>>>>       DECLARE_AIE_MSG(get_telemetry, MSG_OP_GET_TELEMETRY);
>>>>       struct amdxdna_dev *xdna = ndev->aie.xdna;
>>>>       dma_addr_t dma_addr;
>>>> -    u32 buf_sz = size;
>>>> +    u32 buf_sz;
>>>>       u8 *addr;
>>>>       int ret;
>>>>         if (header->type >= MAX_TELEMETRY_TYPE)
>>>>           return -EINVAL;
>>>>   +    buf_sz = min(size, SZ_4M);
>>>>       addr = aie2_alloc_msg_buffer(ndev, &buf_sz, &dma_addr);
>>>>       if (IS_ERR(addr))
>>>>           return PTR_ERR(addr);
>>>> @@ -446,13 +449,14 @@ int aie2_query_telemetry(struct 
>>>> amdxdna_dev_hdl *ndev,
>>>>           goto free_buf;
>>>>       }
>>>>   -    if (size < resp.size) {
>>>> +    if (buf_sz < resp.size) {
>>>>           ret = -EINVAL;
>>>> -        XDNA_ERR(xdna, "Bad buffer size. Available: %u. Needs: 
>>>> %u", size, resp.size);
>>>> +        XDNA_ERR(xdna, "Bad buffer size. Available: %u. Needs: 
>>>> %u", buf_sz, resp.size);
>>>>           goto free_buf;
>>>>       }
>>>>   -    if (copy_to_user(buf, addr, resp.size)) {
>>>> +    size = min(size, resp.size);
>>>> +    if (copy_to_user(buf, addr, size)) {
>>>>           ret = -EFAULT;
>>>>           XDNA_ERR(xdna, "Failed to copy telemetry to user space");
>>>>           goto free_buf;
>>>> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/ 
>>>> amdxdna/aie2_pci.c
>>>> index 164e188ba501..041cbc8cd7e5 100644
>>>> --- a/drivers/accel/amdxdna/aie2_pci.c
>>>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>>>> @@ -620,23 +620,19 @@ static void aie2_fini(struct amdxdna_dev *xdna)
>>>>   static int aie2_get_aie_status(struct amdxdna_client *client,
>>>>                      struct amdxdna_drm_get_info *args)
>>>>   {
>>>> -    struct amdxdna_drm_query_aie_status status;
>>>> +    struct amdxdna_drm_query_aie_status status = {};
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>> +    u32 buf_sz;
>>>>       int ret;
>>>>         ndev = xdna->dev_handle;
>>>> -    if (copy_from_user(&status, u64_to_user_ptr(args->buffer), 
>>>> sizeof(status))) {
>>>> +    buf_sz = min(args->buffer_size, sizeof(status));
>>>> +    if (copy_from_user(&status, u64_to_user_ptr(args->buffer), 
>>>> buf_sz)) {
>>>>           XDNA_ERR(xdna, "Failed to copy AIE request into kernel");
>>>>           return -EFAULT;
>>>>       }
>>>>   -    if (ndev->metadata.cols * ndev->metadata.size < 
>>>> status.buffer_size) {
>>>> -        XDNA_ERR(xdna, "Invalid buffer size. Given Size: %u. Need 
>>>> Size: %u.",
>>>> -             status.buffer_size, ndev->metadata.cols * ndev- 
>>>> >metadata.size);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>>       ret = aie2_query_status(ndev, u64_to_user_ptr(status.buffer),
>>>>                   status.buffer_size, &status.cols_filled);
>>>>       if (ret) {
>>>> @@ -644,7 +640,7 @@ static int aie2_get_aie_status(struct 
>>>> amdxdna_client *client,
>>>>           return ret;
>>>>       }
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &status, 
>>>> sizeof(status))) {
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &status, 
>>>> buf_sz)) {
>>>>           XDNA_ERR(xdna, "Failed to copy AIE request info to user 
>>>> space");
>>>>           return -EFAULT;
>>>>       }
>>>> @@ -659,6 +655,7 @@ static int aie2_get_aie_metadata(struct 
>>>> amdxdna_client *client,
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>>       int ret = 0;
>>>> +    u32 buf_sz;
>>>>         ndev = xdna->dev_handle;
>>>>       meta = kzalloc_obj(*meta);
>>>> @@ -690,7 +687,8 @@ static int aie2_get_aie_metadata(struct 
>>>> amdxdna_client *client,
>>>>       meta->shim.lock_count = ndev->metadata.shim.lock_count;
>>>>       meta->shim.event_reg_count = 
>>>> ndev->metadata.shim.event_reg_count;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), meta, 
>>>> sizeof(*meta)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(*meta));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), meta, buf_sz))
>>>>           ret = -EFAULT;
>>>>         kfree(meta);
>>>> @@ -703,12 +701,14 @@ static int aie2_get_aie_version(struct 
>>>> amdxdna_client *client,
>>>>       struct amdxdna_drm_query_aie_version version;
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>> +    u32 buf_sz;
>>>>         ndev = xdna->dev_handle;
>>>>       version.major = ndev->version.major;
>>>>       version.minor = ndev->version.minor;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &version, 
>>>> sizeof(version)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(version));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &version, 
>>>> buf_sz))
>>>>           return -EFAULT;
>>>>         return 0;
>>>> @@ -719,13 +719,15 @@ static int aie2_get_firmware_version(struct 
>>>> amdxdna_client *client,
>>>>   {
>>>>       struct amdxdna_drm_query_firmware_version version;
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>> +    u32 buf_sz;
>>>>         version.major = xdna->fw_ver.major;
>>>>       version.minor = xdna->fw_ver.minor;
>>>>       version.patch = xdna->fw_ver.sub;
>>>>       version.build = xdna->fw_ver.build;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &version, 
>>>> sizeof(version)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(version));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &version, 
>>>> buf_sz))
>>>>           return -EFAULT;
>>>>         return 0;
>>>> @@ -737,11 +739,13 @@ static int aie2_get_power_mode(struct 
>>>> amdxdna_client *client,
>>>>       struct amdxdna_drm_get_power_mode mode = {};
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>> +    u32 buf_sz;
>>>>         ndev = xdna->dev_handle;
>>>>       mode.power_mode = ndev->pw_mode;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &mode, 
>>>> sizeof(mode)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(mode));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &mode, buf_sz))
>>>>           return -EFAULT;
>>>>         return 0;
>>>> @@ -754,6 +758,7 @@ static int aie2_get_clock_metadata(struct 
>>>> amdxdna_client *client,
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>>       int ret = 0;
>>>> +    u32 buf_sz;
>>>>         ndev = xdna->dev_handle;
>>>>       clock = kzalloc_obj(*clock);
>>>> @@ -766,7 +771,8 @@ static int aie2_get_clock_metadata(struct 
>>>> amdxdna_client *client,
>>>>       snprintf(clock->h_clock.name, sizeof(clock->h_clock.name), "H 
>>>> Clock");
>>>>       clock->h_clock.freq_mhz = ndev->hclk_freq;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), clock, 
>>>> sizeof(*clock)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(*clock));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), clock, buf_sz))
>>>>           ret = -EFAULT;
>>>>         kfree(clock);
>>>> @@ -792,12 +798,14 @@ static int aie2_get_sensors(struct 
>>>> amdxdna_client *client,
>>>>       scnprintf(sensor.label, sizeof(sensor.label), "Total Power");
>>>>       scnprintf(sensor.units, sizeof(sensor.units), "mW");
>>>>   +    if (args->buffer_size < sizeof(sensor))
>>>> +        goto out;
>>>> +
>>>>       if (copy_to_user(u64_to_user_ptr(args->buffer), &sensor, 
>>>> sizeof(sensor)))
>>>>           return -EFAULT;
>>>>   +    args->buffer_size -= sizeof(sensor);
>>>>       sensors_count++;
>>>> -    if (args->buffer_size <= sensors_count * sizeof(sensor))
>>>> -        goto out;
>>>>         for (i = 0; i < min_t(u32, ndev->total_col, 8); i++) {
>>>>           memset(&sensor, 0, sizeof(sensor));
>>>> @@ -807,13 +815,15 @@ static int aie2_get_sensors(struct 
>>>> amdxdna_client *client,
>>>>           scnprintf(sensor.label, sizeof(sensor.label), "Column %d 
>>>> Utilization", i);
>>>>           scnprintf(sensor.units, sizeof(sensor.units), "%%");
>>>>   +        if (args->buffer_size < sizeof(sensor))
>>>> +            goto out;
>>>> +
>>>>           if (copy_to_user(u64_to_user_ptr(args->buffer) + 
>>>> sensors_count * sizeof(sensor),
>>>>                    &sensor, sizeof(sensor)))
>>>>               return -EFAULT;
>>>>   +        args->buffer_size -= sizeof(sensor);
>>>>           sensors_count++;
>>>> -        if (args->buffer_size <= sensors_count * sizeof(sensor))
>>>> -            goto out;
>>>>       }
>>>>     out:
>>>> @@ -909,6 +919,7 @@ static int aie2_query_resource_info(struct 
>>>> amdxdna_client *client,
>>>>       const struct amdxdna_dev_priv *priv;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>>       struct amdxdna_dev *xdna;
>>>> +    u32 buf_sz;
>>>>         xdna = client->xdna;
>>>>       ndev = xdna->dev_handle;
>>>> @@ -920,7 +931,8 @@ static int aie2_query_resource_info(struct 
>>>> amdxdna_client *client,
>>>>       res_info.npu_tops_curr = ndev->curr_tops;
>>>>       res_info.npu_task_curr = ndev->hwctx_num;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &res_info, 
>>>> sizeof(res_info)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(res_info));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &res_info, 
>>>> buf_sz))
>>>>           return -EFAULT;
>>>>         return 0;
>>>> @@ -956,12 +968,7 @@ static int aie2_get_telemetry(struct 
>>>> amdxdna_client *client,
>>>>           XDNA_ERR(xdna, "Invalid buffer size");
>>>>           return -EINVAL;
>>>>       }
>>>> -
>>>>       telemetry_data_sz = args->buffer_size - header_sz;
>>>> -    if (telemetry_data_sz > SZ_4M) {
>>>> -        XDNA_ERR(xdna, "Buffer size is too big, %d", 
>>>> telemetry_data_sz);
>>>> -        return -EINVAL;
>>>> -    }
>>>>         header = kzalloc(header_sz, GFP_KERNEL);
>>>>       if (!header)
>>>> @@ -1002,6 +1009,7 @@ static int aie2_get_preempt_state(struct 
>>>> amdxdna_client *client,
>>>>       struct amdxdna_drm_attribute_state state = {};
>>>>       struct amdxdna_dev *xdna = client->xdna;
>>>>       struct amdxdna_dev_hdl *ndev;
>>>> +    u32 buf_sz;
>>>>         ndev = xdna->dev_handle;
>>>>       if (args->param == DRM_AMDXDNA_GET_FORCE_PREEMPT_STATE)
>>>> @@ -1009,7 +1017,8 @@ static int aie2_get_preempt_state(struct 
>>>> amdxdna_client *client,
>>>>       else if (args->param == 
>>>> DRM_AMDXDNA_GET_FRAME_BOUNDARY_PREEMPT_STATE)
>>>>           state.state = ndev->frame_boundary_preempt;
>>>>   -    if (copy_to_user(u64_to_user_ptr(args->buffer), &state, 
>>>> sizeof(state)))
>>>> +    buf_sz = min(args->buffer_size, sizeof(state));
>>>> +    if (copy_to_user(u64_to_user_ptr(args->buffer), &state, buf_sz))
>>>>           return -EFAULT;
>>>>         return 0;
>>>
>