| Message ID | 20180223192506.29992-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Regards Shashank On 2/24/2018 12:55 AM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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ä <ville.syrjala@linux.intel.com> > --- > 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;
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ä <ville.syrjala@linux.intel.com> > > > > 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ä <ville.syrjala@linux.intel.com> > > --- > > 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;
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ä <ville.syrjala@linux.intel.com> >>> >>> 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ä <ville.syrjala@linux.intel.com> >>> --- >>> 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;
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ä <ville.syrjala@linux.intel.com> > >>> > >>> 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ä <ville.syrjala@linux.intel.com> > >>> --- > >>> 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;
On Fri, Feb 23, 2018 at 09:25:03PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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ä <ville.syrjala@linux.intel.com> > --- > 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 <daniel.vetter@ffwll.ch> 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-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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, &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;