[Intel-gfx] [PATCH] drm: check user mode flags for validity

[Jesse Barnes]

On Thu, 9 May 2013 01:25:59 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Wed, May 08, 2013 at 02:01:25PM -0700, Jesse Barnes wrote:
> > Requested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/drm_crtc.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 792c3e3..72ae33a 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1318,6 +1318,18 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
> >  	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
> >  		return -ERANGE;
> >  
> > +	/* Reject modes with invalid h/vsync */
> > +	if (!(in->flags & (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC)))
> > +		return -EINVAL;
> > +	if ((in->flags & DRM_MODE_FLAG_PHSYNC) &&
> > +	    (in->flags & DRM_MODE_FLAG_NHSYNC))
> > +		return -EINVAL;
> > +	if (!(in->flags & (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC)))
> > +		return -EINVAL;
> > +	if ((in->flags & DRM_MODE_FLAG_PVSYNC) &&
> > +	    (in->flags & DRM_MODE_FLAG_NVSYNC))
> > +		return -EINVAL;
> 
> That might be a bit too drastic. Well I suppose making sure that both
> flags are not enabled at the same time could be OK. But having neither
> flag set could be perfectly legal (the user could be asking for composite
> sync instead for example).
> 
> 
> So my less drastic suggestion would be doing something like this in i915
> specific code:
> 
>  adjusted_mode->flags = 0;
>  if (requested_mode->flags & NHSYNC)
>  	adjusted_mode->flags |= NHSYNC;
>  else
>  	adjusted_mode->flags |= PHSYNC;
> 
> It would gurantee that we end up picking exactly one of the flags in
> every case. If both are set, we pick -, of neither is set we pick +.

Ah yeah I knew I was forgetting something... I'll drop the checks for
no flags.

You really think it would be better to do this in i915?  I guess it's
probably safe, but it seems nicer to filter this out where it might
occur (the EDID quirks should already deal with bogus flags for kernel
generated mode lists).

-- 
Jesse Barnes, Intel Open Source Technology Center