On Wed, May 17, 2017 at 05:26:14PM -0700, Ben Widawsky wrote:
> On 17-05-17 11:17:57, Liviu Dudau wrote:
> > On Tue, May 16, 2017 at 02:31:24PM -0700, Ben Widawsky wrote:
> > > This is the plumbing for supporting fb modifiers on planes. Modifiers
> > > have already been introduced to some extent, but this series will extend
> > > this to allow querying modifiers per plane. Based on this, the client to
> > > enable optimal modifications for framebuffers.
> > >
> > > This patch simply allows the DRM drivers to initialize their list of
> > > supported modifiers upon initializing the plane.
> > >
> > > v2: A minor addition from Daniel
> > >
> > > v3: Updated commit message
> > > s/INVALID/DRM_FORMAT_MOD_INVALID (Liviu)
> > > Remove some excess newlines (Liviu)
> > > Update comment for > 64 modifiers (Liviu)
> > >
> > > Cc: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> > > Reviewed-by: Daniel Stone <daniels@xxxxxxxxxxxxx> (v2)
> > > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> >
> > Minor nits, see below, but otherwise:
> >
> > Reviewed-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> >
> > Thanks,
> > Liviu
> >
>
> Thank you. I took them all but the memcpy change, which I'm pretty sure is okay.
> Take a quick look again and let me know.
[snip]
>
> > > + * format is encoded as a bit and the current code only supports a u64.
> > > + */
> > > + if (WARN_ON(format_count > 64))
> > > + return -EINVAL;
> > > +
> > > + if (format_modifiers) {
> > > + const uint64_t *temp_modifiers = format_modifiers;
> > > + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> > > + format_modifier_count++;
> > > + }
> > > +
> > > + plane->modifier_count = format_modifier_count;
> > > + plane->modifiers = kmalloc_array(format_modifier_count,
> > > + sizeof(format_modifiers[0]),
> > > + GFP_KERNEL);
> > > +
> > > + if (format_modifier_count && !plane->modifiers) {
> > > + DRM_DEBUG_KMS("out of memory when allocating plane\n");
> > > + kfree(plane->format_types);
> > > + drm_mode_object_unregister(dev, &plane->base);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > if (name) {
> > > va_list ap;
> > >
> > > @@ -117,12 +145,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> > > }
> > > if (!plane->name) {
> > > kfree(plane->format_types);
> > > + kfree(plane->modifiers);
> > > drm_mode_object_unregister(dev, &plane->base);
> > > return -ENOMEM;
> > > }
> > >
> > > memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> > > plane->format_count = format_count;
> > > + memcpy(plane->modifiers, format_modifiers,
> > > + format_modifier_count * sizeof(format_modifiers[0]));
> >
> > I'm still worried that we can reach the memcpy with a NULL format_modifiers and we are opening
> > a security hole here.
> >
>
> I didn't notice your feedback here before, I apologize. I'm pretty certain you
> can't get here with !format_modifiers (well you can, but then the 'n' for memcpy
> will be 0). format_modifier_count is only !0 if format_modifiers is !NULL.
That is a valid point. It then makes me ask why we even go through the dance of allocating
a 0 array for plane->modifiers, can we not skip the whole thing around plane->modifiers if
format_modifiers is NULL?
[snip]
> --
> Ben Widawsky, Intel Open Source Technology Center
Thanks for the effort,
Liviu
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
