Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Daniel Vetter]

On Fri, Feb 23, 2018 at 09:25:03PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> While we want to potentially support multiple different gamma/degamma
> LUT sizes we can (and should) at least check that the blob length
> is a multiple of the LUT entry size.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_atomic.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8945357212ba..933edec0299d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct 
> drm_device *dev,
>struct drm_property_blob **blob,
>uint64_t blob_id,
>ssize_t expected_size,
> +  ssize_t expected_size_mod,

Needs kerneldoc, and I'm not sure it's the most descriptive name. Maybe
expected_array_element_size?

With or without the bikeshed, but with the kerneldoc fixed:

Reviewed-by: Daniel Vetter 

Up to this patch in the series.
-Daniel

>bool *replaced)
>  {
>   struct drm_property_blob *new_blob = NULL;
> @@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct 
> drm_device *dev,
>   if (new_blob == NULL)
>   return -EINVAL;
>  
> - if (expected_size > 0 && expected_size != new_blob->length) {
> + if (expected_size > 0 &&
> + new_blob->length != expected_size) {
> + drm_property_blob_put(new_blob);
> + return -EINVAL;
> + }
> + if (expected_size_mod > 0 &&
> + new_blob->length % expected_size_mod != 0) {
>   drm_property_blob_put(new_blob);
>   return -EINVAL;
>   }
> @@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   ret = drm_atomic_replace_property_blob_from_id(dev,
>   >degamma_lut,
>   val,
> - -1,
> + -1, sizeof(struct drm_color_lut),
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> @@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   ret = drm_atomic_replace_property_blob_from_id(dev,
>   >ctm,
>   val,
> - sizeof(struct drm_color_ctm),
> + sizeof(struct drm_color_ctm), -1,
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> @@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   ret = drm_atomic_replace_property_blob_from_id(dev,
>   >gamma_lut,
>   val,
> - -1,
> + -1, sizeof(struct drm_color_lut),
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> -- 
> 2.13.6
> 
> ___
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 07:58:07PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/1/2018 6:54 PM, Ville Syrjälä wrote:
> > On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 2/24/2018 12:55 AM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> While we want to potentially support multiple different gamma/degamma
> >>> LUT sizes we can (and should) at least check that the blob length
> >>> is a multiple of the LUT entry size.
> >> I dint understand the exact idea behind doing this, how is this going to
> >> benefit ? May be a bit more description ?
> > The benefit is rejecting garbage fed in from userspace.
> >
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>drivers/gpu/drm/drm_atomic.c | 15 +++
> >>>1 file changed, 11 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 8945357212ba..933edec0299d 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct 
> >>> drm_device *dev,
> >>>struct drm_property_blob 
> >>> **blob,
> >>>uint64_t blob_id,
> >>>ssize_t expected_size,
> >>> +  ssize_t expected_size_mod,
> >>>bool *replaced)
> >>>{
> >>>   struct drm_property_blob *new_blob = NULL;
> >>> @@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct 
> >>> drm_device *dev,
> >>>   if (new_blob == NULL)
> >>>   return -EINVAL;
> >>>
> >>> - if (expected_size > 0 && expected_size != new_blob->length) {
> >>> + if (expected_size > 0 &&
> >>> + new_blob->length != expected_size) {
> >>> + drm_property_blob_put(new_blob);
> >>> + return -EINVAL;
> >>> + }
> >> One line needed here, matching the previous if() pattern
> > What line? Don't understand.
> I mean, I can see a blank line before previous if() condition, so lets 
> keep the same pattern for this if() too

OTOH the two ifs are related so maybe just keep them together? Doesn't
actually matter to me though.

> 
> - Shashank
> >>> + if (expected_size_mod > 0 &&
> >>> + new_blob->length % expected_size_mod != 0) {
> >>>   drm_property_blob_put(new_blob);
> >>>   return -EINVAL;
> >>>   }
> >>> @@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc 
> >>> *crtc,
> >>>   ret = drm_atomic_replace_property_blob_from_id(dev,
> >>>   >degamma_lut,
> >>>   val,
> >>> - -1,
> >>> + -1, sizeof(struct drm_color_lut),
> >>>   );
> >>>   state->color_mgmt_changed |= replaced;
> >>>   return ret;
> >>> @@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc 
> >>> *crtc,
> >>>   ret = drm_atomic_replace_property_blob_from_id(dev,
> >>>   >ctm,
> >>>   val,
> >>> - sizeof(struct drm_color_ctm),
> >>> + sizeof(struct drm_color_ctm), -1,
> >>>   );
> >>>   state->color_mgmt_changed |= replaced;
> >>>   return ret;
> >>> @@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc 
> >>> *crtc,
> >>>   ret = drm_atomic_replace_property_blob_from_id(dev,
> >>>   >gamma_lut,
> >>>   val,
> >>> - -1,
> >>> + -1, sizeof(struct drm_color_lut),
> >>>   );
> >>>   state->color_mgmt_changed |= replaced;
> >>>   return ret;

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Sharma, Shashank]

Regards

Shashank


On 3/1/2018 6:54 PM, Ville Syrjälä wrote:

On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 2/24/2018 12:55 AM, Ville Syrjala wrote:

From: Ville Syrjälä 

While we want to potentially support multiple different gamma/degamma
LUT sizes we can (and should) at least check that the blob length
is a multiple of the LUT entry size.

I dint understand the exact idea behind doing this, how is this going to
benefit ? May be a bit more description ?

The benefit is rejecting garbage fed in from userspace.


Signed-off-by: Ville Syrjälä 
---
   drivers/gpu/drm/drm_atomic.c | 15 +++
   1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8945357212ba..933edec0299d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
 struct drm_property_blob **blob,
 uint64_t blob_id,
 ssize_t expected_size,
+ssize_t expected_size_mod,
 bool *replaced)
   {
struct drm_property_blob *new_blob = NULL;
@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
if (new_blob == NULL)
return -EINVAL;
   
-		if (expected_size > 0 && expected_size != new_blob->length) {

+   if (expected_size > 0 &&
+   new_blob->length != expected_size) {
+   drm_property_blob_put(new_blob);
+   return -EINVAL;
+   }

One line needed here, matching the previous if() pattern

What line? Don't understand.
I mean, I can see a blank line before previous if() condition, so lets 
keep the same pattern for this if() too


- Shashank

+   if (expected_size_mod > 0 &&
+   new_blob->length % expected_size_mod != 0) {
drm_property_blob_put(new_blob);
return -EINVAL;
}
@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>degamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>ctm,
val,
-   sizeof(struct drm_color_ctm),
+   sizeof(struct drm_color_ctm), -1,
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>gamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;


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

Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/24/2018 12:55 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> >
> > While we want to potentially support multiple different gamma/degamma
> > LUT sizes we can (and should) at least check that the blob length
> > is a multiple of the LUT entry size.
> I dint understand the exact idea behind doing this, how is this going to 
> benefit ? May be a bit more description ?

The benefit is rejecting garbage fed in from userspace.

> >
> > Signed-off-by: Ville Syrjälä 
> > ---
> >   drivers/gpu/drm/drm_atomic.c | 15 +++
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 8945357212ba..933edec0299d 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct 
> > drm_device *dev,
> >  struct drm_property_blob **blob,
> >  uint64_t blob_id,
> >  ssize_t expected_size,
> > +ssize_t expected_size_mod,
> >  bool *replaced)
> >   {
> > struct drm_property_blob *new_blob = NULL;
> > @@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct 
> > drm_device *dev,
> > if (new_blob == NULL)
> > return -EINVAL;
> >   
> > -   if (expected_size > 0 && expected_size != new_blob->length) {
> > +   if (expected_size > 0 &&
> > +   new_blob->length != expected_size) {
> > +   drm_property_blob_put(new_blob);
> > +   return -EINVAL;
> > +   }
> One line needed here, matching the previous if() pattern

What line? Don't understand.

> > +   if (expected_size_mod > 0 &&
> > +   new_blob->length % expected_size_mod != 0) {
> > drm_property_blob_put(new_blob);
> > return -EINVAL;
> > }
> > @@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > ret = drm_atomic_replace_property_blob_from_id(dev,
> > >degamma_lut,
> > val,
> > -   -1,
> > +   -1, sizeof(struct drm_color_lut),
> > );
> > state->color_mgmt_changed |= replaced;
> > return ret;
> > @@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > ret = drm_atomic_replace_property_blob_from_id(dev,
> > >ctm,
> > val,
> > -   sizeof(struct drm_color_ctm),
> > +   sizeof(struct drm_color_ctm), -1,
> > );
> > state->color_mgmt_changed |= replaced;
> > return ret;
> > @@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > ret = drm_atomic_replace_property_blob_from_id(dev,
> > >gamma_lut,
> > val,
> > -   -1,
> > +   -1, sizeof(struct drm_color_lut),
> > );
> > state->color_mgmt_changed |= replaced;
> > return ret;

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Sharma, Shashank]

Regards

Shashank


On 2/24/2018 12:55 AM, Ville Syrjala wrote:

From: Ville Syrjälä 

While we want to potentially support multiple different gamma/degamma
LUT sizes we can (and should) at least check that the blob length
is a multiple of the LUT entry size.
I dint understand the exact idea behind doing this, how is this going to 
benefit ? May be a bit more description ?


Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/drm_atomic.c | 15 +++
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8945357212ba..933edec0299d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
 struct drm_property_blob **blob,
 uint64_t blob_id,
 ssize_t expected_size,
+ssize_t expected_size_mod,
 bool *replaced)
  {
struct drm_property_blob *new_blob = NULL;
@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
if (new_blob == NULL)
return -EINVAL;
  
-		if (expected_size > 0 && expected_size != new_blob->length) {

+   if (expected_size > 0 &&
+   new_blob->length != expected_size) {
+   drm_property_blob_put(new_blob);
+   return -EINVAL;
+   }

One line needed here, matching the previous if() pattern

+   if (expected_size_mod > 0 &&
+   new_blob->length % expected_size_mod != 0) {
drm_property_blob_put(new_blob);
return -EINVAL;
}
@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>degamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>ctm,
val,
-   sizeof(struct drm_color_ctm),
+   sizeof(struct drm_color_ctm), -1,
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>gamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;


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

[Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size

[Ville Syrjala]

From: Ville Syrjälä 

While we want to potentially support multiple different gamma/degamma
LUT sizes we can (and should) at least check that the blob length
is a multiple of the LUT entry size.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_atomic.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8945357212ba..933edec0299d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
 struct drm_property_blob **blob,
 uint64_t blob_id,
 ssize_t expected_size,
+ssize_t expected_size_mod,
 bool *replaced)
 {
struct drm_property_blob *new_blob = NULL;
@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device 
*dev,
if (new_blob == NULL)
return -EINVAL;
 
-   if (expected_size > 0 && expected_size != new_blob->length) {
+   if (expected_size > 0 &&
+   new_blob->length != expected_size) {
+   drm_property_blob_put(new_blob);
+   return -EINVAL;
+   }
+   if (expected_size_mod > 0 &&
+   new_blob->length % expected_size_mod != 0) {
drm_property_blob_put(new_blob);
return -EINVAL;
}
@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>degamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>ctm,
val,
-   sizeof(struct drm_color_ctm),
+   sizeof(struct drm_color_ctm), -1,
);
state->color_mgmt_changed |= replaced;
return ret;
@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
ret = drm_atomic_replace_property_blob_from_id(dev,
>gamma_lut,
val,
-   -1,
+   -1, sizeof(struct drm_color_lut),
);
state->color_mgmt_changed |= replaced;
return ret;
-- 
2.13.6

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