| Message ID | 1428096466-27622-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6130
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -2 272/272 270/272
ILK 302/302 302/302
SNB 303/303 303/303
IVB 338/338 338/338
BYT -1 287/287 286/287
HSW 361/361 361/361
BDW 308/308 308/308
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
PNV igt@gem_tiled_pread_pwrite FAIL(3)PASS(11) FAIL(1)PASS(1)
PNV igt@gem_userptr_blits@coherency-sync CRASH(5)PASS(8) CRASH(1)PASS(1)
*BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(18) FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
On Fri, Apr 03, 2015 at 02:27:46PM -0700, Matt Roper wrote: > Add tests for destination rectangle integer overflow before calling the > driver's check function. This will ensure that the transitional plane > helpers match the behavior of the full atomic helpers by always > returning -ERANGE for planes positioned beyond INT_MAX. > > Note that the legacy SetPlane ioctl itself also includes similar tests > for integer overflow, so the only case where this check really matters > is when legacy cursor operations get routed through the universal plane > interface internally. > > This issue was first noticed with i915 commit: > > commit ff42e093e9c9c17a6e1d6aab24875a36795f926e > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Mon Mar 2 16:35:20 2015 +0100 > > Revert "drm/i915: Switch planes from transitional helpers to full > atomic helpers" > > The above revert switched us from full atomic helpers back to the > transitional helpers, and in doing so we lost the overflow checking here > for universal cursor updates. Even though such extreme cursor positions > are unlikely to actually happen in the wild, we still don't want there > to be a change of behavior when drivers switch from transitional helpers > to full helpers. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269 > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Since this is just for the cursor ioctl ... shouldn't we instead move these checks from drm_mode_setplane to __setplane_internal? That way other drivers also can rely upon these guarantees when implementing universal cursor support and we won't have a mismatch in what kind of plane properties can sneak through to drivers. -Daniel > --- > drivers/gpu/drm/drm_plane_helper.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index 33807e0..1e9e105 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -417,6 +417,20 @@ int drm_plane_helper_commit(struct drm_plane *plane, > for (i = 0; i < 2; i++) > crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL; > > + /* > + * Give drivers some help against integer overflows (and match the > + * behavior of the full atomic helpers). > + */ > + if (plane_state->crtc_w > INT_MAX || > + plane_state->crtc_x > INT_MAX - (int32_t) plane_state->crtc_w || > + plane_state->crtc_h > INT_MAX || > + plane_state->crtc_y > INT_MAX - (int32_t) plane_state->crtc_h) { > + DRM_DEBUG_ATOMIC("Invalid CRTC coordinates %ux%u+%d+%d\n", > + plane_state->crtc_w, plane_state->crtc_h, > + plane_state->crtc_x, plane_state->crtc_y); > + return -ERANGE; > + } > + > if (plane_funcs->atomic_check) { > ret = plane_funcs->atomic_check(plane, plane_state); > if (ret) > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 33807e0..1e9e105 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -417,6 +417,20 @@ int drm_plane_helper_commit(struct drm_plane *plane, for (i = 0; i < 2; i++) crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL; + /* + * Give drivers some help against integer overflows (and match the + * behavior of the full atomic helpers). + */ + if (plane_state->crtc_w > INT_MAX || + plane_state->crtc_x > INT_MAX - (int32_t) plane_state->crtc_w || + plane_state->crtc_h > INT_MAX || + plane_state->crtc_y > INT_MAX - (int32_t) plane_state->crtc_h) { + DRM_DEBUG_ATOMIC("Invalid CRTC coordinates %ux%u+%d+%d\n", + plane_state->crtc_w, plane_state->crtc_h, + plane_state->crtc_x, plane_state->crtc_y); + return -ERANGE; + } + if (plane_funcs->atomic_check) { ret = plane_funcs->atomic_check(plane, plane_state); if (ret)
Add tests for destination rectangle integer overflow before calling the driver's check function. This will ensure that the transitional plane helpers match the behavior of the full atomic helpers by always returning -ERANGE for planes positioned beyond INT_MAX. Note that the legacy SetPlane ioctl itself also includes similar tests for integer overflow, so the only case where this check really matters is when legacy cursor operations get routed through the universal plane interface internally. This issue was first noticed with i915 commit: commit ff42e093e9c9c17a6e1d6aab24875a36795f926e Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Mon Mar 2 16:35:20 2015 +0100 Revert "drm/i915: Switch planes from transitional helpers to full atomic helpers" The above revert switched us from full atomic helpers back to the transitional helpers, and in doing so we lost the overflow checking here for universal cursor updates. Even though such extreme cursor positions are unlikely to actually happen in the wild, we still don't want there to be a change of behavior when drivers switch from transitional helpers to full helpers. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269 Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_plane_helper.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)