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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel