[PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware

[Chris Wilson]

On Tue, Oct 01, 2013 at 06:49:42PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 01, 2013 at 02:06:13PM +0100, Chris Wilson wrote:
> >  CC      drivers/gpu/drm/drm_edid_load.o
> > drivers/gpu/drm/drm_edid_load.c: In function ‘drm_load_edid_firmware’: include/linux/err.h:39:17: warning: ‘edid’ may be used uninitialised in this function [-Wuninitialized]
> > drivers/gpu/drm/drm_edid_load.c:141:22: note: ‘edid’ was declared here
> > 
> > In the process, we can make the error handling more resilient.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/drm_edid_load.c |   75 +++++++++++++++++----------------------
> >  1 file changed, 32 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> > index 271b42b..4b57a4c 100644
> > --- a/drivers/gpu/drm/drm_edid_load.c
> > +++ b/drivers/gpu/drm/drm_edid_load.c
> > @@ -136,59 +136,51 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
> >  static u8 *edid_load(struct drm_connector *connector, const char *name,
> >  			const char *connector_name)
> >  {
> > -	const struct firmware *fw;
> > +	const struct firmware *fw = NULL;
> >  	struct platform_device *pdev;
> > -	u8 *fwdata = NULL, *edid, *new_edid;
> > -	int fwsize, expected;
> > -	int builtin = 0, err = 0;
> > +	u8 *fwdata, *edid;
> 
> Orthogonal issue, but fwdata, generic_edid and generic_edid_names could
> all be const.
> 
> > +	int fwsize, expected, err, builtin;
> >  	int i, valid_extensions = 0;
> >  	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> >  
> >  	pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> > -	if (IS_ERR(pdev)) {
> > -		DRM_ERROR("Failed to register EDID firmware platform device "
> > -		    "for connector \"%s\"\n", connector_name);
> > -		err = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > -	err = request_firmware(&fw, name, &pdev->dev);
> > -	platform_device_unregister(pdev);
> > +	if (!IS_ERR(pdev)) {
> > +		err = request_firmware(&fw, name, &pdev->dev);
> > +		platform_device_unregister(pdev);
> > +	} else
> > +		err = PTR_ERR(pdev);
> >  
> > -	if (err) {
> > +	if (err == 0) {
> > +		fwdata = (u8 *)fw->data;
> > +		fwsize = fw->size;
> > +		builtin = 0;
> > +	} else {
> >  		i = 0;
> >  		while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
> >  			i++;
> > -		if (i < GENERIC_EDIDS) {
> > -			err = 0;
> > -			builtin = 1;
> > -			fwdata = generic_edid[i];
> > -			fwsize = sizeof(generic_edid[i]);
> > +		if (i >= GENERIC_EDIDS) {
> > +			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> > +					name, err);
> > +			edid = ERR_PTR(err);
> > +			goto out;
> 
> Due to the 'if (fw)' check in the cleanup code, you could eliminate
> the out label.
> 
> >  		}
> > -	}
> >  
> > -	if (err) {
> > -		DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> > -		    name, err);
> > -		goto out;
> > -	}
> > -
> > -	if (fwdata == NULL) {
> > -		fwdata = (u8 *) fw->data;
> > -		fwsize = fw->size;
> > +		fwdata = generic_edid[i];
> > +		fwsize = sizeof(generic_edid[i]);
> > +		builtin = 1;
> >  	}
> >  
> >  	expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
> 
> Not your bug, but we're missing a check for fwsize > 0x7e.
> 
> Can't spot any real bugs, so w/ or w/o the out label idea:
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Might as well spend the time to fix up the little warts whilst we are
here, so expect a v2 shortly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre