[PATCH] drm/atomic: Reject properties not part of the object.

[PATCH] drm/atomic: Reject properties not part of the object.

[Maarten Lankhorst]

The legacy setprop ioctl doesn't attempt to set properties
that are not enumerated on the object. The atomic ioctl does,
fix this by validating first.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5cb2e22d5d55..a5126e5c05ee 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_crtc_state *crtc_state;
 	unsigned plane_mask;
 	int ret = 0;
-	unsigned int i, j;
+	unsigned int i, j, k;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1691,6 +1691,15 @@ retry:
 				goto out;
 			}
 
+			for (k = 0; k < obj->properties->count; k++)
+				if (obj->properties->properties[k]->base.id == prop_id)
+					break;
+
+			if (k == obj->properties->count) {
+				ret = -EINVAL;
+				goto out;
+			}
+
 			prop = drm_property_find(dev, prop_id);
 			if (!prop) {
 				drm_mode_object_unreference(obj);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Move property validation to a helper.

[Maarten Lankhorst]

Property lifetimes are equal to the device lifetime, so the separate
drm_property_find is not needed. The pointer can be retrieved from
the properties member, which saves us some locking and a extra lookup.

The lifetime for properties is until the device is destroyed, which
happens late in the device unload path.

Testcase: kms_properties
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 16 ++++------------
 drivers/gpu/drm/drm_crtc_internal.h |  2 ++
 drivers/gpu/drm/drm_mode_object.c   | 31 ++++++++++++++++---------------
 3 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fac156c43506..8bec8466781c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_crtc_state *crtc_state;
 	unsigned plane_mask;
 	int ret = 0;
-	unsigned int i, j, k;
+	unsigned int i, j;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1691,19 +1691,11 @@ retry:
 				goto out;
 			}
 
-			for (k = 0; k < obj->properties->count; k++)
-				if (obj->properties->properties[k]->base.id == prop_id)
-					break;
-
-			if (k == obj->properties->count) {
-				ret = -EINVAL;
-				goto out;
-			}
-
-			prop = drm_property_find(dev, prop_id);
+			prop = drm_mode_obj_find_prop_id(obj, prop_id);
 			if (!prop) {
 				drm_mode_object_unreference(obj);
-				ret = -ENOENT;
+				DRM_DEBUG_ATOMIC("cannot find property %u for obj %u\n", prop_id, obj_id);
+				ret = -EINVAL;
 				goto out;
 			}
 
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index a3622644bccf..444e609078cc 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -115,6 +115,8 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic,
 				   uint32_t __user *prop_ptr,
 				   uint64_t __user *prop_values,
 				   uint32_t *arg_count_props);
+struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
+					       uint32_t prop_id);
 
 /* IOCTL */
 
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 6edda8382a4c..9f17085b1fdd 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -372,14 +372,25 @@ out:
 	return ret;
 }
 
+struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
+					       uint32_t prop_id)
+{
+	int i;
+
+	for (i = 0; i < obj->properties->count; i++)
+		if (obj->properties->properties[i]->base.id == prop_id)
+			return obj->properties->properties[i];
+
+	return NULL;
+}
+
 int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
 {
 	struct drm_mode_obj_set_property *arg = data;
 	struct drm_mode_object *arg_obj;
-	struct drm_mode_object *prop_obj;
 	struct drm_property *property;
-	int i, ret = -EINVAL;
+	int ret = -EINVAL;
 	struct drm_mode_object *ref;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
@@ -392,23 +403,13 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		ret = -ENOENT;
 		goto out;
 	}
-	if (!arg_obj->properties)
-		goto out_unref;
-
-	for (i = 0; i < arg_obj->properties->count; i++)
-		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
-			break;
 
-	if (i == arg_obj->properties->count)
+	if (!arg_obj->properties)
 		goto out_unref;
 
-	prop_obj = drm_mode_object_find(dev, arg->prop_id,
-					DRM_MODE_OBJECT_PROPERTY);
-	if (!prop_obj) {
-		ret = -ENOENT;
+	property = drm_mode_obj_find_prop_id(arg_obj, arg->prop_id);
+	if (!property)
 		goto out_unref;
-	}
-	property = obj_to_property(prop_obj);
 
 	if (!drm_property_change_valid_get(property, arg->value, &ref))
 		goto out_unref;
-- 
2.7.4


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Move property validation to a helper.

[Maarten Lankhorst]

Op 07-09-16 om 10:58 schreef Maarten Lankhorst:
> Property lifetimes are equal to the device lifetime, so the separate
> drm_property_find is not needed. The pointer can be retrieved from
> the properties member, which saves us some locking and a extra lookup.
>
> The lifetime for properties is until the device is destroyed, which
> happens late in the device unload path.
>
> Testcase: kms_properties
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 16 ++++------------
>  drivers/gpu/drm/drm_crtc_internal.h |  2 ++
>  drivers/gpu/drm/drm_mode_object.c   | 31 ++++++++++++++++---------------
>  3 files changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fac156c43506..8bec8466781c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	struct drm_crtc_state *crtc_state;
>  	unsigned plane_mask;
>  	int ret = 0;
> -	unsigned int i, j, k;
> +	unsigned int i, j;
>  
>  	/* disallow for drivers not supporting atomic: */
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1691,19 +1691,11 @@ retry:
>  				goto out;
>  			}
>  
> -			for (k = 0; k < obj->properties->count; k++)
> -				if (obj->properties->properties[k]->base.id == prop_id)
> -					break;
> -
> -			if (k == obj->properties->count) {
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -
> -			prop = drm_property_find(dev, prop_id);
> +			prop = drm_mode_obj_find_prop_id(obj, prop_id);
>  			if (!prop) {
>  				drm_mode_object_unreference(obj);
> -				ret = -ENOENT;
> +				DRM_DEBUG_ATOMIC("cannot find property %u for obj %u\n", prop_id, obj_id);
> +				ret = -EINVAL;
This needs to stay -ENOENT it seems. kms_atomic testcase relies on it.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm: Move property validation to a helper, v2.

[Maarten Lankhorst]

Property lifetimes are equal to the device lifetime, so the separate
drm_property_find is not needed. The pointer can be retrieved from
the properties member, which saves us some locking and a extra lookup.
The lifetime for properties is until the device is destroyed, which
happens late in the device unload path.

kms_atomic is also testing for invalid properties which returns -ENOENT,
to be consistent return -ENOENT for valid properties that don't appear
on the object property list.

Changes since v1:
- Return -ENOENT for invalid properties to make kms_atomic pass.
- Change commit message slightly to take this into account.

Testcase: kms_atomic
Testcase: kms_properties
Fixes: 4e9951d96093 ("drm/atomic: Reject properties not part of the object.")
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 13 ++-----------
 drivers/gpu/drm/drm_crtc_internal.h |  2 ++
 drivers/gpu/drm/drm_mode_object.c   | 31 ++++++++++++++++---------------
 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index fac156c43506..23739609427d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_crtc_state *crtc_state;
 	unsigned plane_mask;
 	int ret = 0;
-	unsigned int i, j, k;
+	unsigned int i, j;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1691,16 +1691,7 @@ retry:
 				goto out;
 			}
 
-			for (k = 0; k < obj->properties->count; k++)
-				if (obj->properties->properties[k]->base.id == prop_id)
-					break;
-
-			if (k == obj->properties->count) {
-				ret = -EINVAL;
-				goto out;
-			}
-
-			prop = drm_property_find(dev, prop_id);
+			prop = drm_mode_obj_find_prop_id(obj, prop_id);
 			if (!prop) {
 				drm_mode_object_unreference(obj);
 				ret = -ENOENT;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index a3622644bccf..444e609078cc 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -115,6 +115,8 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic,
 				   uint32_t __user *prop_ptr,
 				   uint64_t __user *prop_values,
 				   uint32_t *arg_count_props);
+struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
+					       uint32_t prop_id);
 
 /* IOCTL */
 
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 6edda8382a4c..9f17085b1fdd 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -372,14 +372,25 @@ out:
 	return ret;
 }
 
+struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
+					       uint32_t prop_id)
+{
+	int i;
+
+	for (i = 0; i < obj->properties->count; i++)
+		if (obj->properties->properties[i]->base.id == prop_id)
+			return obj->properties->properties[i];
+
+	return NULL;
+}
+
 int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
 {
 	struct drm_mode_obj_set_property *arg = data;
 	struct drm_mode_object *arg_obj;
-	struct drm_mode_object *prop_obj;
 	struct drm_property *property;
-	int i, ret = -EINVAL;
+	int ret = -EINVAL;
 	struct drm_mode_object *ref;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
@@ -392,23 +403,13 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		ret = -ENOENT;
 		goto out;
 	}
-	if (!arg_obj->properties)
-		goto out_unref;
-
-	for (i = 0; i < arg_obj->properties->count; i++)
-		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
-			break;
 
-	if (i == arg_obj->properties->count)
+	if (!arg_obj->properties)
 		goto out_unref;
 
-	prop_obj = drm_mode_object_find(dev, arg->prop_id,
-					DRM_MODE_OBJECT_PROPERTY);
-	if (!prop_obj) {
-		ret = -ENOENT;
+	property = drm_mode_obj_find_prop_id(arg_obj, arg->prop_id);
+	if (!property)
 		goto out_unref;
-	}
-	property = obj_to_property(prop_obj);
 
 	if (!drm_property_change_valid_get(property, arg->value, &ref))
 		goto out_unref;
-- 
2.7.4


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm: Move property validation to a helper, v2.

[Sean Paul]

On Thu, Sep 8, 2016 at 6:30 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Property lifetimes are equal to the device lifetime, so the separate
> drm_property_find is not needed. The pointer can be retrieved from
> the properties member, which saves us some locking and a extra lookup.
> The lifetime for properties is until the device is destroyed, which
> happens late in the device unload path.
>
> kms_atomic is also testing for invalid properties which returns -ENOENT,
> to be consistent return -ENOENT for valid properties that don't appear
> on the object property list.
>
> Changes since v1:
> - Return -ENOENT for invalid properties to make kms_atomic pass.
> - Change commit message slightly to take this into account.
>
> Testcase: kms_atomic
> Testcase: kms_properties
> Fixes: 4e9951d96093 ("drm/atomic: Reject properties not part of the object.")
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Nice cleanup! applied to drm-misc

Sean

> ---
>  drivers/gpu/drm/drm_atomic.c        | 13 ++-----------
>  drivers/gpu/drm/drm_crtc_internal.h |  2 ++
>  drivers/gpu/drm/drm_mode_object.c   | 31 ++++++++++++++++---------------
>  3 files changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fac156c43506..23739609427d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1609,7 +1609,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>         struct drm_crtc_state *crtc_state;
>         unsigned plane_mask;
>         int ret = 0;
> -       unsigned int i, j, k;
> +       unsigned int i, j;
>
>         /* disallow for drivers not supporting atomic: */
>         if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1691,16 +1691,7 @@ retry:
>                                 goto out;
>                         }
>
> -                       for (k = 0; k < obj->properties->count; k++)
> -                               if (obj->properties->properties[k]->base.id == prop_id)
> -                                       break;
> -
> -                       if (k == obj->properties->count) {
> -                               ret = -EINVAL;
> -                               goto out;
> -                       }
> -
> -                       prop = drm_property_find(dev, prop_id);
> +                       prop = drm_mode_obj_find_prop_id(obj, prop_id);
>                         if (!prop) {
>                                 drm_mode_object_unreference(obj);
>                                 ret = -ENOENT;
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index a3622644bccf..444e609078cc 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -115,6 +115,8 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic,
>                                    uint32_t __user *prop_ptr,
>                                    uint64_t __user *prop_values,
>                                    uint32_t *arg_count_props);
> +struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
> +                                              uint32_t prop_id);
>
>  /* IOCTL */
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 6edda8382a4c..9f17085b1fdd 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -372,14 +372,25 @@ out:
>         return ret;
>  }
>
> +struct drm_property *drm_mode_obj_find_prop_id(struct drm_mode_object *obj,
> +                                              uint32_t prop_id)
> +{
> +       int i;
> +
> +       for (i = 0; i < obj->properties->count; i++)
> +               if (obj->properties->properties[i]->base.id == prop_id)
> +                       return obj->properties->properties[i];
> +
> +       return NULL;
> +}
> +
>  int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>                                     struct drm_file *file_priv)
>  {
>         struct drm_mode_obj_set_property *arg = data;
>         struct drm_mode_object *arg_obj;
> -       struct drm_mode_object *prop_obj;
>         struct drm_property *property;
> -       int i, ret = -EINVAL;
> +       int ret = -EINVAL;
>         struct drm_mode_object *ref;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
> @@ -392,23 +403,13 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>                 ret = -ENOENT;
>                 goto out;
>         }
> -       if (!arg_obj->properties)
> -               goto out_unref;
> -
> -       for (i = 0; i < arg_obj->properties->count; i++)
> -               if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
> -                       break;
>
> -       if (i == arg_obj->properties->count)
> +       if (!arg_obj->properties)
>                 goto out_unref;
>
> -       prop_obj = drm_mode_object_find(dev, arg->prop_id,
> -                                       DRM_MODE_OBJECT_PROPERTY);
> -       if (!prop_obj) {
> -               ret = -ENOENT;
> +       property = drm_mode_obj_find_prop_id(arg_obj, arg->prop_id);
> +       if (!property)
>                 goto out_unref;
> -       }
> -       property = obj_to_property(prop_obj);
>
>         if (!drm_property_change_valid_get(property, arg->value, &ref))
>                 goto out_unref;
> --
> 2.7.4
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel