| Message ID | 1415193919-1687-7-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 05, 2014 at 02:25:18PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB > IOCTL, only the width, height, bpp and flags fields are inputs. The > caller is not guaranteed to zero out or set handle, pitch and size. > Drivers must not treat these values as possible inputs, otherwise they > may use uninitialized memory during the computation of the framebuffer > size. > > The R-Car DU driver treats the pitch passed in from userspace as minimum > and will only overwrite it when the driver-computed pitch is larger, > allowing userspace to, intentionally or not, overallocate framebuffers. > > Signed-off-by: Thierry Reding <treding@nvidia.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Aside: I tend to put the maintainer Cc: into the patch so that they're on the permanent record. That will also make sure that when this patch gets resend (e.g. stable backport) maintainers are still included properly. -Daniel > --- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > index 5329491e32c3..6289e3797bc5 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev, > else > align = 16 * args->bpp / 8; > > - args->pitch = roundup(max(args->pitch, min_pitch), align); > + args->pitch = roundup(min_pitch, align); > > return drm_gem_cma_dumb_create_internal(file, dev, args); > } > -- > 2.1.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thierry, Thank you for the patch. On Wednesday 05 November 2014 14:25:18 Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB > IOCTL, only the width, height, bpp and flags fields are inputs. The > caller is not guaranteed to zero out or set handle, pitch and size. > Drivers must not treat these values as possible inputs, otherwise they > may use uninitialized memory during the computation of the framebuffer > size. > > The R-Car DU driver treats the pitch passed in from userspace as minimum > and will only overwrite it when the driver-computed pitch is larger, > allowing userspace to, intentionally or not, overallocate framebuffers. As discussed on IRC, my concern with this is that some userspace applications might be relying on this behaviour. I'm not personally aware of any though, so I'm not opposed to this patch, but I can't vouch it won't cause any userspace breakage. (On a side note I believe treating the pitch and size arguments as inputs could be a worthwhile extension to the API, but given that we haven't rejected incorrect values in the past we're pretty much stuck). > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 5329491e32c3..6289e3797bc5 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct > drm_device *dev, else > align = 16 * args->bpp / 8; > > - args->pitch = roundup(max(args->pitch, min_pitch), align); > + args->pitch = roundup(min_pitch, align); > > return drm_gem_cma_dumb_create_internal(file, dev, args); > }
On Wed, Nov 05, 2014 at 08:47:07PM +0200, Laurent Pinchart wrote: > (On a side note I believe treating the pitch and size arguments as inputs > could be a worthwhile extension to the API, but given that we haven't > rejected incorrect values in the past we're pretty much stuck). The bigger picture here is that it is a generic interface to DRM drivers, and having different behaviour from different drivers is itself impractical. If nothing breaks through better enforcement of the API, then we should go with what the API is currently defined to be, rather than trying to overload it with new features - and then end up in the situation where we have something trying to use those new features and no way to know if the driver implements this "bug" or not.
Hi Russell, On Thursday 06 November 2014 00:54:51 Russell King - ARM Linux wrote: > On Wed, Nov 05, 2014 at 08:47:07PM +0200, Laurent Pinchart wrote: > > (On a side note I believe treating the pitch and size arguments as inputs > > could be a worthwhile extension to the API, but given that we haven't > > rejected incorrect values in the past we're pretty much stuck). > > The bigger picture here is that it is a generic interface to DRM > drivers, and having different behaviour from different drivers is > itself impractical. I totally agree with you, I just believe it would have been nice to be able to extend the API that way, but that's no big deal. Consistency is definitely important. > If nothing breaks through better enforcement of the API, then we > should go with what the API is currently defined to be, rather than > trying to overload it with new features - and then end up in the > situation where we have something trying to use those new features > and no way to know if the driver implements this "bug" or not.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 5329491e32c3..6289e3797bc5 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev, else align = 16 * args->bpp / 8; - args->pitch = roundup(max(args->pitch, min_pitch), align); + args->pitch = roundup(min_pitch, align); return drm_gem_cma_dumb_create_internal(file, dev, args); }