From: Ville Syrjälä ville.syrjala@linux.intel.com
Creating a property that doesn't have a name makes no sense to me. Don't allow it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..fe8627fb7ae6 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -99,10 +99,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list);
- if (name) { - strncpy(property->name, name, DRM_PROP_NAME_LEN); - property->name[DRM_PROP_NAME_LEN-1] = '\0'; - } + strncpy(property->name, name, DRM_PROP_NAME_LEN); + property->name[DRM_PROP_NAME_LEN-1] = '\0';
list_add_tail(&property->head, &dev->mode_config.property_list);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Reject requests to add properties/enums with an overly long name. Previously we would have just silently truncated the string and exposed it userspace.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index fe8627fb7ae6..f062adf21b9c 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -78,6 +78,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, struct drm_property *property = NULL; int ret;
+ if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) + return -EINVAL; + property = kzalloc(sizeof(struct drm_property), GFP_KERNEL); if (!property) return NULL; @@ -372,6 +375,9 @@ int drm_property_add_enum(struct drm_property *property, int index, { struct drm_property_enum *prop_enum;
+ if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) + return -EINVAL; + if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) return -EINVAL;
From: Ville Syrjälä ville.syrjala@linux.intel.com
Reject requests to add properties/enums with an overly long name. Previously we would have just silently truncated the string and exposed it userspace.
v2: drm_property_create() returns a pointer
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index fe8627fb7ae6..c37ac41125b5 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -78,6 +78,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, struct drm_property *property = NULL; int ret;
+ if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) + return NULL; + property = kzalloc(sizeof(struct drm_property), GFP_KERNEL); if (!property) return NULL; @@ -372,6 +375,9 @@ int drm_property_add_enum(struct drm_property *property, int index, { struct drm_property_enum *prop_enum;
+ if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) + return -EINVAL; + if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) return -EINVAL;
On Fri, Mar 02, 2018 at 04:03:00PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Reject requests to add properties/enums with an overly long name. Previously we would have just silently truncated the string and exposed it userspace.
v2: drm_property_create() returns a pointer
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
While you're typing validation code, I noticed that drm_property_add_enum allows you to overwrite the name of an existing enum value already added. That sounds like something we never want to allow either.
Anyway, this is
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_property.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index fe8627fb7ae6..c37ac41125b5 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -78,6 +78,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, struct drm_property *property = NULL; int ret;
- if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
return NULL;- property = kzalloc(sizeof(struct drm_property), GFP_KERNEL); if (!property) return NULL;
@@ -372,6 +375,9 @@ int drm_property_add_enum(struct drm_property *property, int index, { struct drm_property_enum *prop_enum;
- if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
return -EINVAL;- if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) return -EINVAL;
-- 2.16.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 06, 2018 at 11:22:54AM +0100, Daniel Vetter wrote:
On Fri, Mar 02, 2018 at 04:03:00PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Reject requests to add properties/enums with an overly long name. Previously we would have just silently truncated the string and exposed it userspace.
v2: drm_property_create() returns a pointer
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
While you're typing validation code, I noticed that drm_property_add_enum allows you to overwrite the name of an existing enum value already added. That sounds like something we never want to allow either.
Good point. I suppose we should just WARN and error out there as well?
Anyway, this is
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_property.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index fe8627fb7ae6..c37ac41125b5 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -78,6 +78,9 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, struct drm_property *property = NULL; int ret;
- if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
return NULL;- property = kzalloc(sizeof(struct drm_property), GFP_KERNEL); if (!property) return NULL;
@@ -372,6 +375,9 @@ int drm_property_add_enum(struct drm_property *property, int index, { struct drm_property_enum *prop_enum;
- if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
return -EINVAL;- if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK))) return -EINVAL;
-- 2.16.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
BT.2020 specifies two YCbCr<->RGB conversion formulas. The more traditional non-constant luminance and a more complicate constant luminance one. Add an enum value for the constant luminance variant as well in case someone has hardware supporting it.
v2: Reduce the enum name to fit within the 31 character uapi limit
Cc: Harry Wentland harry.wentland@amd.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Daniel Stone daniel@fooishbar.org Cc: Russell King - ARM Linux linux@armlinux.org.uk Cc: Ilia Mirkin imirkin@alum.mit.edu Cc: Hans Verkuil hverkuil@xs4all.nl CC: Uma Shankar uma.shankar@intel.com Cc: Shashank Sharma shashank.sharma@intel.com Cc: Jyri Sarha jsarha@ti.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Acked-by: Harry Wentland harry.wentland@amd.com #v1 --- drivers/gpu/drm/drm_color_mgmt.c | 1 + include/drm/drm_color_mgmt.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 4ff064623836..cc28c32399af 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -358,6 +358,7 @@ static const char * const color_encoding_name[] = { [DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr", [DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr", [DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr", + [DRM_COLOR_YCBCR_BT2020_CONST] = "ITU-R BT.2020 YCbCr const", };
static const char * const color_range_name[] = { diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index b3b6d302ca8c..175943c87d5b 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -41,7 +41,8 @@ int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, enum drm_color_encoding { DRM_COLOR_YCBCR_BT601, DRM_COLOR_YCBCR_BT709, - DRM_COLOR_YCBCR_BT2020, + DRM_COLOR_YCBCR_BT2020, /* non-constant luminance */ + DRM_COLOR_YCBCR_BT2020_CONST, /* constant luminance */ DRM_COLOR_ENCODING_MAX, };
On Fri, Mar 02, 2018 at 03:25:42PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Creating a property that doesn't have a name makes no sense to me. Don't allow it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_property.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..fe8627fb7ae6 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -99,10 +99,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list);
- if (name) {
strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';- }
strncpy(property->name, name, DRM_PROP_NAME_LEN);
property->name[DRM_PROP_NAME_LEN-1] = '\0';
list_add_tail(&property->head, &dev->mode_config.property_list);
-- 2.13.6
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ville,
On 2 March 2018 at 13:25, Ville Syrjala ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Creating a property that doesn't have a name makes no sense to me. Don't allow it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_property.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..fe8627fb7ae6 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -99,10 +99,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list);
if (name) {strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';}
strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';
Shouldn't one also a) error (WARN + return NULL) out when name is NULL and b) update the documentation?
-Emil
On Tue, Mar 06, 2018 at 01:35:11PM +0000, Emil Velikov wrote:
Hi Ville,
On 2 March 2018 at 13:25, Ville Syrjala ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Creating a property that doesn't have a name makes no sense to me. Don't allow it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_property.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..fe8627fb7ae6 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -99,10 +99,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list);
if (name) {strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';}
strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';Shouldn't one also a) error (WARN + return NULL) out when name is NULL and b) update the documentation?
The developer gets the oops. No point in further handholding IMO.
Didn't look at the docs us usual. Are they saying NULL is OK?
On 6 March 2018 at 13:54, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Mar 06, 2018 at 01:35:11PM +0000, Emil Velikov wrote:
Hi Ville,
On 2 March 2018 at 13:25, Ville Syrjala ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Creating a property that doesn't have a name makes no sense to me. Don't allow it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_property.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..fe8627fb7ae6 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -99,10 +99,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list);
if (name) {strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';}
strncpy(property->name, name, DRM_PROP_NAME_LEN);property->name[DRM_PROP_NAME_LEN-1] = '\0';Shouldn't one also a) error (WARN + return NULL) out when name is NULL and b) update the documentation?
The developer gets the oops. No point in further handholding IMO.
Gut feeling suggests that not everyone tests all their drm_properly_create codepaths. So it's more of user gets an oops. But that's another story ;-)
Didn't look at the docs us usual. Are they saying NULL is OK?
The drm_property_create does not explicitly mention (allow?) NULL. Yet again I don't recall any instances of the docs saying that, using NULL is allowed/valid.
It's been a while since I read through the docs, so they might be updated.
Either way it was a simple fly-by suggestion. -Emil
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Ville Syrjala -
Ville Syrjälä