diff mbox

drm: Prevent use of uninitialised values whilst loading edid firmware

Message ID 1380632773-15183-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 1, 2013, 1:06 p.m. UTC
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(-)

Comments

Ville Syrjala Oct. 1, 2013, 3:49 p.m. UTC | #1
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
Chris Wilson Oct. 1, 2013, 4:22 p.m. UTC | #2
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@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>

Might as well spend the time to fix up the little warts whilst we are
here, so expect a v2 shortly.
-Chris
Jani Nikula Oct. 2, 2013, 7:52 a.m. UTC | #3
On Tue, 01 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 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@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>
>
> Might as well spend the time to fix up the little warts whilst we are
> here, so expect a v2 shortly.

One thing that bugs me about the current code is that with
CONFIG_FW_LOADER_USER_HELPER=y, if the firmware isn't builtin and the
kernel can't load the firmware directly from the filesystem, it will
take a full minute to timeout if userspace/udev isn't ready yet. This
usually happens when the user is trying to use the DRM builtin EDIDs,
and a user has reported this happening.

request_firmware() tries to load the firmware in order:
1) kernel builtin - fw_get_builtin_firmware()
2) kernel direct load - fw_get_filesystem_firmware()
3) usermode helper - fw_load_from_user_helper()

Given the above order, I don't think it would be unreasonable to move
the DRM builtin EDID loading to the top of the list, especially since 3)
is prone to take a long time in early boot. An alternative would be to
use request_firmware_nowait(), but that seems like it could get messy.

I don't know if that fits in with what you're doing, or whether you'd
like to do that, but it's something to think about while at it.

BR,
Jani.
diff mbox

Patch

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;
+	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;
 		}
-	}
 
-	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;
 	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;
 }