Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size
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, > &state->degamma_lut, > val, > - -1, > + -1, sizeof(struct drm_color_lut), > &replaced); > 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, > &state->ctm, > val, > - sizeof(struct drm_color_ctm), > + sizeof(struct drm_color_ctm), -1, > &replaced); > 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, > &state->gamma_lut, > val, > - -1, > + -1, sizeof(struct drm_color_lut), > &replaced); > state->color_mgmt_changed |= replaced; > return ret; > -- > 2.13.6 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size
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, > >>> &state->degamma_lut, > >>> val, > >>> - -1, > >>> + -1, sizeof(struct drm_color_lut), > >>> &replaced); > >>> 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, > >>> &state->ctm, > >>> val, > >>> - sizeof(struct drm_color_ctm), > >>> + sizeof(struct drm_color_ctm), -1, > >>> &replaced); > >>> 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, > >>> &state->gamma_lut, > >>> val, > >>> - -1, > >>> + -1, sizeof(struct drm_color_lut), > >>> &replaced); > >>> state->color_mgmt_changed |= replaced; > >>> return ret; -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size
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, &state->degamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); 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, &state->ctm, val, - sizeof(struct drm_color_ctm), + sizeof(struct drm_color_ctm), -1, &replaced); 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, &state->gamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size
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, > > &state->degamma_lut, > > val, > > - -1, > > + -1, sizeof(struct drm_color_lut), > > &replaced); > > 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, > > &state->ctm, > > val, > > - sizeof(struct drm_color_ctm), > > + sizeof(struct drm_color_ctm), -1, > > &replaced); > > 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, > > &state->gamma_lut, > > val, > > - -1, > > + -1, sizeof(struct drm_color_lut), > > &replaced); > > state->color_mgmt_changed |= replaced; > > return ret; -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 3/6] drm: Verify gamma/degamma LUT size
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, &state->degamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); 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, &state->ctm, val, - sizeof(struct drm_color_ctm), + sizeof(struct drm_color_ctm), -1, &replaced); 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, &state->gamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel