diff mbox

[1/9] drm: Warn about negative sizes when calculating scale factor

Message ID 1469549224-1860-2-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä July 26, 2016, 4:06 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Passing negative width/hight to scale factor calculations is not
legal. Let's WARN if that happens.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_rect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chris Wilson July 26, 2016, 4:24 p.m. UTC | #1 
On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Passing negative width/hight to scale factor calculations is not
> legal. Let's WARN if that happens.

Does this get called with user controllable inputs? A quick grep leads
me to drm_primary_helper_update() which suggests no. Did I miss a
potential user controllable WARN->panic?
-Chris
Ville Syrjälä July 26, 2016, 4:39 p.m. UTC | #2 
On Tue, Jul 26, 2016 at 05:24:42PM +0100, Chris Wilson wrote:
> On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Passing negative width/hight to scale factor calculations is not
> > legal. Let's WARN if that happens.
> 
> Does this get called with user controllable inputs?

User controllable to a degree. width/height can only ever be positive
though.

> A quick grep leads
> me to drm_primary_helper_update() which suggests no. Did I miss a
> potential user controllable WARN->panic?

I just landed in the BUG_ON in intel_sprite.c on account of a typo I
made in the user src/crtc coordinate -> drm_rect conversion. Should
probably replace the BUG_ON() with WARN_ON() in i915 as well...
Sean Paul July 29, 2016, 8:41 p.m. UTC | #3 
On Tue, Jul 26, 2016 at 12:39 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Jul 26, 2016 at 05:24:42PM +0100, Chris Wilson wrote:
>> On Tue, Jul 26, 2016 at 07:06:56PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Passing negative width/hight to scale factor calculations is not

nit: s/hight/height/

>> > legal. Let's WARN if that happens.
>>
>> Does this get called with user controllable inputs?
>
> User controllable to a degree. width/height can only ever be positive
> though.
>

I think the only risk is getting UINT_MAX from userspace, since
drm_rect stores ints. However, it looks like check_src_coords() and
the check in __setplane_internal() should ensure those values are
pruned out.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>> A quick grep leads
>> me to drm_primary_helper_update() which suggests no. Did I miss a
>> potential user controllable WARN->panic?
>
> I just landed in the BUG_ON in intel_sprite.c on account of a typo I
> made in the user src/crtc coordinate -> drm_rect conversion. Should
> probably replace the BUG_ON() with WARN_ON() in i915 as well...
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index a8e2c8603945..512199b60728 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -100,7 +100,7 @@  static int drm_calc_scale(int src, int dst)
 {
 	int scale = 0;
 
-	if (src < 0 || dst < 0)
+	if (WARN_ON(src < 0 || dst < 0))
 		return -EINVAL;
 
 	if (dst == 0)