[1/8] drm/fb: More paranoia in addfb checks
diff mbox series

Message ID 20191115092120.4445-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • fb_create drive-through cleanups
Related show

Commit Message

Daniel Vetter Nov. 15, 2019, 9:21 a.m. UTC
- Our limit is uint32_t, make that explicit.

- Untangle the one overflow check, I think (but not sure) that with
  all three together you could overflow the uint64_t and it'd look
  cool again. Hence two steps. Also go with the more common (and imo
  safer approach) of reducing the range we accept, instead of trying
  to compute the overflow in high enough precision.

- The above would blow up if we get a 0 pitches, so check for that
  too, but only if block_size is a thing.

Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Pekka Paalanen Nov. 15, 2019, 10:49 a.m. UTC | #1
On Fri, 15 Nov 2019 10:21:13 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> - Our limit is uint32_t, make that explicit.
> 
> - Untangle the one overflow check, I think (but not sure) that with
>   all three together you could overflow the uint64_t and it'd look
>   cool again. Hence two steps. Also go with the more common (and imo
>   safer approach) of reducing the range we accept, instead of trying
>   to compute the overflow in high enough precision.
> 
> - The above would blow up if we get a 0 pitches, so check for that
>   too, but only if block_size is a thing.
> 
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 57564318ceea..3141c6ed6dd2 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -214,15 +214,20 @@ static int framebuffer_check(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		if (min_pitch > UINT_MAX)
> +		if (min_pitch > U8_MAX)

This looks odd, but I don't know what min_pitch is or why it should be
limited to 255(?). What's with the U8, should it not be U32?

>  			return -ERANGE;
>  
> -		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> -			return -ERANGE;
> +		if (block_size) {
> +			if (r->pitches[i] < min_pitch) {
> +				DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> +				return -EINVAL;
> +			}
>  
> -		if (block_size && r->pitches[i] < min_pitch) {
> -			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> -			return -EINVAL;
> +			if (height > U8_MAX / r->pitches[i])
> +				return -ERANGE;
> +
> +			if (r->offsets[i] > U8_MAX / r->pitches[i] - height)

Aside from the U8 again, this looks strange too. You want to check that
offset + height * pitch does not exceed MAX?

Wouldn't that be height > (MAX - offset) / pitch for bad?

If offset cannot be negative, this could also replace height > U8_MAX /
r->pitches[i].

> +				return -ERANGE;
>  		}
>  
>  		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {

Thanks,
pq
Ville Syrjälä Nov. 15, 2019, 12:44 p.m. UTC | #2
On Fri, Nov 15, 2019 at 10:21:13AM +0100, Daniel Vetter wrote:
> - Our limit is uint32_t, make that explicit.
> 
> - Untangle the one overflow check, I think (but not sure) that with
>   all three together you could overflow the uint64_t and it'd look
>   cool again.

It can't overflow. All theree inputs are u32 so the max value
you can get is 0xffffffff00000000.

> Hence two steps. Also go with the more common (and imo
>   safer approach) 

Also results in multiple divisions which is needlessly expensive.
The original is just mul+add.

> of reducing the range we accept, instead of trying
>   to compute the overflow in high enough precision.
> 
> - The above would blow up if we get a 0 pitches, so check for that
>   too, but only if block_size is a thing.
> 
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 57564318ceea..3141c6ed6dd2 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -214,15 +214,20 @@ static int framebuffer_check(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		if (min_pitch > UINT_MAX)
> +		if (min_pitch > U8_MAX)

U8?

>  			return -ERANGE;
>  
> -		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> -			return -ERANGE;
> +		if (block_size) {
> +			if (r->pitches[i] < min_pitch) {
> +				DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> +				return -EINVAL;
> +			}
>  
> -		if (block_size && r->pitches[i] < min_pitch) {
> -			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> -			return -EINVAL;
> +			if (height > U8_MAX / r->pitches[i])
> +				return -ERANGE;
> +
> +			if (r->offsets[i] > U8_MAX / r->pitches[i] - height)
> +				return -ERANGE;
>  		}
>  
>  		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> -- 
> 2.24.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 15, 2019, 3:26 p.m. UTC | #3
On Fri, Nov 15, 2019 at 1:44 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Nov 15, 2019 at 10:21:13AM +0100, Daniel Vetter wrote:
> > - Our limit is uint32_t, make that explicit.
> >
> > - Untangle the one overflow check, I think (but not sure) that with
> >   all three together you could overflow the uint64_t and it'd look
> >   cool again.
>
> It can't overflow. All theree inputs are u32 so the max value
> you can get is 0xffffffff00000000.

Hm right, I just checked, I guess I should have beforehand.

> > Hence two steps. Also go with the more common (and imo
> >   safer approach)
>
> Also results in multiple divisions which is needlessly expensive.
> The original is just mul+add.
>
> > of reducing the range we accept, instead of trying
> >   to compute the overflow in high enough precision.
> >
> > - The above would blow up if we get a 0 pitches, so check for that
> >   too, but only if block_size is a thing.
> >
> > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 57564318ceea..3141c6ed6dd2 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -214,15 +214,20 @@ static int framebuffer_check(struct drm_device *dev,
> >                       return -EINVAL;
> >               }
> >
> > -             if (min_pitch > UINT_MAX)
> > +             if (min_pitch > U8_MAX)
>
> U8?

Oh dear, some patch editing gone really wrong. I think I'll drop this
one here, not doing any good :-)
-Daniel

>
> >                       return -ERANGE;
> >
> > -             if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> > -                     return -ERANGE;
> > +             if (block_size) {
> > +                     if (r->pitches[i] < min_pitch) {
> > +                             DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > +                             return -EINVAL;
> > +                     }
> >
> > -             if (block_size && r->pitches[i] < min_pitch) {
> > -                     DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > -                     return -EINVAL;
> > +                     if (height > U8_MAX / r->pitches[i])
> > +                             return -ERANGE;
> > +
> > +                     if (r->offsets[i] > U8_MAX / r->pitches[i] - height)
> > +                             return -ERANGE;
> >               }
> >
> >               if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> > --
> > 2.24.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 57564318ceea..3141c6ed6dd2 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -214,15 +214,20 @@  static int framebuffer_check(struct drm_device *dev,
 			return -EINVAL;
 		}
 
-		if (min_pitch > UINT_MAX)
+		if (min_pitch > U8_MAX)
 			return -ERANGE;
 
-		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
-			return -ERANGE;
+		if (block_size) {
+			if (r->pitches[i] < min_pitch) {
+				DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
+				return -EINVAL;
+			}
 
-		if (block_size && r->pitches[i] < min_pitch) {
-			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
-			return -EINVAL;
+			if (height > U8_MAX / r->pitches[i])
+				return -ERANGE;
+
+			if (r->offsets[i] > U8_MAX / r->pitches[i] - height)
+				return -ERANGE;
 		}
 
 		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {