[Intel-gfx] [PATCH v3 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

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 at arm.com>
> > > Reviewed-by: Daniel Stone <daniels at collabora.com> (v2)
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > 
> > Minor nits, see below, but otherwise:
> > 
> > Reviewed-by: Liviu Dudau <Liviu.Dudau at arm.com>
> > 
> > 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!  /
  ---------------
    ¯\_(ツ)_/¯