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

[Mario Limonciello <mario.limonciello@amd.com>]

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

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?

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

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