Re: [PATCH RESEND 2/3] drm/atomic: add max_size check to drm_property_replace_blob_from_id()

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 10.12.25 um 20:42 schrieb Dmitry Baryshkov:
> The function drm_property_replace_blob_from_id() allows checking whether
> the blob size is equal to a predefined value. In case of variable-size
> properties (like the gamma / degamma LUTs) we might want to check for
> the blob size against the maximum, allowing properties of the size
> lesser than the max supported by the hardware. Extend the function in
> order to support such checks.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c |  5 +++++
>   drivers/gpu/drm/drm_atomic_uapi.c                       |  7 +++++--
>   drivers/gpu/drm/drm_property.c                          | 11 +++++++++++
>   include/drm/drm_property.h                              |  1 +
>   4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index e027798ece03..d19631b5d9e1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1677,6 +1677,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   							&dm_plane_state->degamma_lut,
>   							val, -1,
>   							sizeof(struct drm_color_lut),
> +							0,

The rest of the API uses -1 for an invalid/unknown argument. I'd stick 
with that instead of using 0.

>   							&replaced);
>   		dm_plane_state->base.color_mgmt_changed |= replaced;
>   		return ret;
> @@ -1695,6 +1696,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   							&dm_plane_state->ctm,
>   							val,
>   							sizeof(struct drm_color_ctm_3x4), -1,
> +							0,
>   							&replaced);
>   		dm_plane_state->base.color_mgmt_changed |= replaced;
>   		return ret;
> @@ -1703,6 +1705,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   							&dm_plane_state->shaper_lut,
>   							val, -1,
>   							sizeof(struct drm_color_lut),
> +							0,
>   							&replaced);
>   		dm_plane_state->base.color_mgmt_changed |= replaced;
>   		return ret;
> @@ -1716,6 +1719,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   							&dm_plane_state->lut3d,
>   							val, -1,
>   							sizeof(struct drm_color_lut),
> +							0,
>   							&replaced);
>   		dm_plane_state->base.color_mgmt_changed |= replaced;
>   		return ret;
> @@ -1724,6 +1728,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   							&dm_plane_state->blend_lut,
>   							val, -1,
>   							sizeof(struct drm_color_lut),
> +							0,
>   							&replaced);
>   		dm_plane_state->base.color_mgmt_changed |= replaced;
>   		return ret;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 85dbdaa4a2e2..7d2076f1006e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -388,6 +388,7 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   					&state->degamma_lut,
>   					val,
>   					-1, sizeof(struct drm_color_lut),
> +					0,
>   					&replaced);
>   		state->color_mgmt_changed |= replaced;
>   		return ret;
> @@ -395,7 +396,7 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   		ret = drm_property_replace_blob_from_id(dev,
>   					&state->ctm,
>   					val,
> -					sizeof(struct drm_color_ctm), -1,
> +					sizeof(struct drm_color_ctm), -1, 0,
>   					&replaced);
>   		state->color_mgmt_changed |= replaced;
>   		return ret;
> @@ -404,6 +405,7 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   					&state->gamma_lut,
>   					val,
>   					-1, sizeof(struct drm_color_lut),
> +					0,
>   					&replaced);
>   		state->color_mgmt_changed |= replaced;
>   		return ret;
> @@ -546,6 +548,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   					val,
>   					-1,
>   					sizeof(struct drm_mode_rect),
> +					0,
>   					&replaced);
>   		return ret;
>   	} else if (property == plane->scaling_filter_property) {
> @@ -741,7 +744,7 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   		ret = drm_property_replace_blob_from_id(dev,
>   				&state->hdr_output_metadata,
>   				val,
> -				sizeof(struct hdr_output_metadata), -1,
> +				sizeof(struct hdr_output_metadata), -1, 0,
>   				&replaced);
>   		return ret;
>   	} else if (property == config->aspect_ratio_property) {
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index 596272149a35..5befe443135d 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -759,6 +759,7 @@ EXPORT_SYMBOL(drm_property_replace_blob);
>    * @blob_id: the id of the new blob to replace with
>    * @expected_size: expected size of the blob property
>    * @expected_elem_size: expected size of an element in the blob property
> + * @max_size: the maximum size of the blob property for variable-size blobs
>    * @replaced: if the blob was in fact replaced
>    *
>    * Look up the new blob from id, take its reference, check expected sizes of
> @@ -773,6 +774,7 @@ int drm_property_replace_blob_from_id(struct drm_device *dev,
>   					 uint64_t blob_id,
>   					 ssize_t expected_size,
>   					 ssize_t expected_elem_size,
> +					 ssize_t max_size,

I'd put max_size before expected_size, so that the size of individual 
elements isn't located in the middle of them.

>   					 bool *replaced)
>   {
>   	struct drm_property_blob *new_blob = NULL;
> @@ -801,6 +803,15 @@ int drm_property_replace_blob_from_id(struct drm_device *dev,
>   			drm_property_blob_put(new_blob);
>   			return -EINVAL;
>   		}
> +
> +		if (max_size > 0 &&
> +		    new_blob->length > max_size) {
> +			drm_dbg_atomic(dev,
> +				       "[BLOB:%d] length %zu greater than max %zu\n",
> +				       new_blob->base.id, new_blob->length, max_size);
> +			drm_property_blob_put(new_blob);
> +			return -EINVAL;
> +		}

I'd first test for max_size before testing for expected size.

And shouldn't you also test for (max_size % expected_elem_size == 0)?

Best regards
Thomas

>   	}
>   
>   	*replaced |= drm_property_replace_blob(blob, new_blob);
> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
> index 082f29156b3e..aa49b5a42bb5 100644
> --- a/include/drm/drm_property.h
> +++ b/include/drm/drm_property.h
> @@ -284,6 +284,7 @@ int drm_property_replace_blob_from_id(struct drm_device *dev,
>   				      uint64_t blob_id,
>   				      ssize_t expected_size,
>   				      ssize_t expected_elem_size,
> +				      ssize_t max_size,
>   				      bool *replaced);
>   int drm_property_replace_global_blob(struct drm_device *dev,
>   				     struct drm_property_blob **replace,
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)