| Message ID | 20201213183930.349592-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/6] drm/damage_helper: Check if damage clips has valid values | expand |
> Userspace can set a damage clip with a negative coordinate, negative > width or height or larger than the plane. > This invalid values could cause issues in some HW or even worst enable > security flaws. > > v2: > - add debug messages to let userspace know why atomic commit failed > due invalid damage clips > > Cc: Simon Ser <contact@emersion.fr> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Deepak Rawat <drawat@vmware.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> After looking at the kernel code, it seems like the kernel already checks for all of that in drm_atomic_plane_check. Are you aware of this? > + w = drm_rect_width(&plane_state->src) >> 16; > + h = drm_rect_height(&plane_state->src) >> 16; The docs say this should be in FB coordinates, not in SRC_* coordinates. So we shouldn't need to check any SRC_* prop here.
On Mon, 2020-12-14 at 08:55 +0000, Simon Ser wrote: > > Userspace can set a damage clip with a negative coordinate, > > negative > > width or height or larger than the plane. > > This invalid values could cause issues in some HW or even worst > > enable > > security flaws. > > > > v2: > > - add debug messages to let userspace know why atomic commit failed > > due invalid damage clips > > > > Cc: Simon Ser <contact@emersion.fr> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Fabio Estevam <festevam@gmail.com> > > Cc: Deepak Rawat <drawat@vmware.com> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > After looking at the kernel code, it seems like the kernel already > checks for > all of that in drm_atomic_plane_check. Are you aware of this? > > > + w = drm_rect_width(&plane_state->src) >> 16; > > + h = drm_rect_height(&plane_state->src) >> 16; > > The docs say this should be in FB coordinates, not in SRC_* > coordinates. So we > shouldn't need to check any SRC_* prop here. > I agree the Simon's opinion. it does check between plane's frame buffer src geometry and damage clips. (Plane's damage clip might exist outside of fb src geometry.)
On Mon, Dec 14, 2020 at 09:27:30AM +0000, Mun, Gwan-gyeong wrote: > On Mon, 2020-12-14 at 08:55 +0000, Simon Ser wrote: > > > Userspace can set a damage clip with a negative coordinate, > > > negative > > > width or height or larger than the plane. > > > This invalid values could cause issues in some HW or even worst > > > enable > > > security flaws. > > > > > > v2: > > > - add debug messages to let userspace know why atomic commit failed > > > due invalid damage clips > > > > > > Cc: Simon Ser <contact@emersion.fr> > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Cc: Sean Paul <seanpaul@chromium.org> > > > Cc: Fabio Estevam <festevam@gmail.com> > > > Cc: Deepak Rawat <drawat@vmware.com> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > After looking at the kernel code, it seems like the kernel already > > checks for > > all of that in drm_atomic_plane_check. Are you aware of this? > > > > > + w = drm_rect_width(&plane_state->src) >> 16; > > > + h = drm_rect_height(&plane_state->src) >> 16; > > > > The docs say this should be in FB coordinates, not in SRC_* > > coordinates. So we > > shouldn't need to check any SRC_* prop here. > > > I agree the Simon's opinion. it does check between plane's frame buffer > src geometry and damage clips. (Plane's damage clip might exist outside > of fb src geometry.) Since this is causing confusion, please make sure that the igt for damage clips validates this correctly. I think some of the igts that vmwgfx people have created have still not yet landed, so we definitely want to land these. Note that basic damage clips tests should be fully generic, i.e. not tied to anything driver specific like our psr testcases are. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..c6b341ecae2c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -897,7 +897,9 @@ drm_atomic_helper_check_planes(struct drm_device *dev, drm_atomic_helper_plane_changed(state, old_plane_state, new_plane_state, plane); - drm_atomic_helper_check_plane_damage(state, new_plane_state); + ret = drm_atomic_helper_check_plane_damage(state, new_plane_state); + if (ret) + return ret; if (!funcs || !funcs->atomic_check) continue; diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index 3a4126dc2520..69a557aaa8cf 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -33,6 +33,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_damage_helper.h> #include <drm/drm_device.h> +#include <drm/drm_print.h> /** * DOC: overview @@ -104,36 +105,76 @@ void drm_plane_enable_fb_damage_clips(struct drm_plane *plane) EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips); /** - * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check. + * drm_atomic_helper_check_plane_damage - Verify plane damage clips on + * atomic_check. * @state: The driver state object. - * @plane_state: Plane state for which to verify damage. + * @plane_state: Plane state for which to verify damage clips. * - * This helper function makes sure that damage from plane state is discarded - * for full modeset. If there are more reasons a driver would want to do a full - * plane update rather than processing individual damage regions, then those - * cases should be taken care of here. + * This helper checks if all damage clips has valid values and makes sure that + * damage clips from plane state is discarded for full modeset. If there are + * more reasons a driver would want to do a full plane update rather than + * processing individual damage regions, then those cases should be taken care + * of here. * * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that * full plane update should happen. It also ensure helper iterator will return * &drm_plane_state.src as damage. + * + * Return: Zero on success, negative errno on failure. */ -void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state, - struct drm_plane_state *plane_state) +int drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state, + struct drm_plane_state *plane_state) { + struct drm_mode_rect *damage_clips; struct drm_crtc_state *crtc_state; + unsigned int num_clips, w, h; + + num_clips = drm_plane_get_damage_clips_count(plane_state); + if (!num_clips) + return 0; if (plane_state->crtc) { crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc); if (WARN_ON(!crtc_state)) - return; + return 0; if (drm_atomic_crtc_needs_modeset(crtc_state)) { drm_property_blob_put(plane_state->fb_damage_clips); plane_state->fb_damage_clips = NULL; + return 0; + } + } + + w = drm_rect_width(&plane_state->src) >> 16; + h = drm_rect_height(&plane_state->src) >> 16; + damage_clips = drm_plane_get_damage_clips(plane_state); + + for (; num_clips; num_clips--, damage_clips++) { + if (damage_clips->x1 < 0 || damage_clips->x2 < 0 || + damage_clips->y1 < 0 || damage_clips->y2 < 0) { + drm_dbg_atomic(state->dev, + "Invalid damage clip, negative coordinate\n"); + return -EINVAL; + } + + if (damage_clips->x2 < damage_clips->x1 || + damage_clips->y2 < damage_clips->y1) { + drm_dbg_atomic(state->dev, + "Invalid damage clip, negative width or height\n"); + return -EINVAL; + } + + if ((damage_clips->x2 - damage_clips->x1) > w || + (damage_clips->y2 - damage_clips->y1) > h) { + drm_dbg_atomic(state->dev, + "Invalid damage clip, width or height larger than plane\n"); + return -EINVAL; } } + + return 0; } EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage); diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index 40c34a5bf149..5e344d1a2b22 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -65,8 +65,8 @@ struct drm_atomic_helper_damage_iter { }; void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); -void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state, - struct drm_plane_state *plane_state); +int drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state, + struct drm_plane_state *plane_state); int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, struct drm_file *file_priv, unsigned int flags, unsigned int color, struct drm_clip_rect *clips,
Userspace can set a damage clip with a negative coordinate, negative width or height or larger than the plane. This invalid values could cause issues in some HW or even worst enable security flaws. v2: - add debug messages to let userspace know why atomic commit failed due invalid damage clips Cc: Simon Ser <contact@emersion.fr> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Fabio Estevam <festevam@gmail.com> Cc: Deepak Rawat <drawat@vmware.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 +- drivers/gpu/drm/drm_damage_helper.c | 59 ++++++++++++++++++++++++----- include/drm/drm_damage_helper.h | 4 +- 3 files changed, 55 insertions(+), 12 deletions(-)