| Message ID | 20191115092120.4445-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fb_create drive-through cleanups | expand |
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
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
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
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)) {
- 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(-)