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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@linux.intel.com>

>  	if (expected != fwsize) {
>  		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
>  		    "(expected %d, got %d)\n", name, expected, (int) fwsize);
> -		err = -EINVAL;
> +		edid = ERR_PTR(-EINVAL);
>  		goto relfw_out;
>  	}
>  
>  	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
>  	if (edid == NULL) {
> -		err = -ENOMEM;
> +		edid = ERR_PTR(-ENOMEM);
>  		goto relfw_out;
>  	}
>  
> @@ -197,7 +189,7 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
>  		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
>  		    name);
>  		kfree(edid);
> -		err = -EINVAL;
> +		edid = ERR_PTR(-EINVAL);
>  		goto relfw_out;
>  	}
>  
> @@ -210,19 +202,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
>  	}
>  
>  	if (valid_extensions != edid[0x7e]) {
> +		u8 *new_edid;
> +
>  		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>  		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
>  		    "\"%s\" for connector \"%s\"\n", valid_extensions,
>  		    edid[0x7e], name, connector_name);
>  		edid[0x7e] = valid_extensions;
> +
>  		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> -		    GFP_KERNEL);
> -		if (new_edid == NULL) {
> -			err = -ENOMEM;
> -			kfree(edid);
> -			goto relfw_out;
> -		}
> -		edid = new_edid;
> +				    GFP_KERNEL);
> +		if (new_edid)
> +			edid = new_edid;
>  	}
>  
>  	DRM_INFO("Got %s EDID base block and %d extension%s from "
> @@ -231,12 +222,10 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
>  	    name, connector_name);
>  
>  relfw_out:
> -	release_firmware(fw);
> +	if (fw)
> +		release_firmware(fw);
>  
>  out:
> -	if (err)
> -		return ERR_PTR(err);
> -
>  	return edid;
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel