Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare <jdelvare@suse.de>]

On Fri, 6 Aug 2021 12:47:00 +0200, Pali Rohár wrote:
> On Friday 06 August 2021 11:55:19 Jean Delvare wrote:
> > Therefore I would propose the following alternative:
> > 
> > --- linux-5.13.orig/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:11:44.275200299 +0200
> > +++ linux-5.13/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:18:19.847469822 +0200
> > @@ -1194,7 +1194,7 @@ static acpi_status check_acpi_smo88xx_de
> >  
> >  	kfree(info);
> >  
> > -	*((bool *)return_value) = true;
> > +	*return_value = hid;	/* Could be any address, used as true value */
> >  	return AE_CTRL_TERMINATE;
> >  
> >  smo88xx_not_found:
> > @@ -1204,13 +1204,10 @@ static acpi_status check_acpi_smo88xx_de
> >  
> >  static bool is_dell_system_with_lis3lv02d(void)
> >  {
> > -	bool found;
> > -	const char *vendor;
> > +	acpi_handle found = NULL;  
> 
> Then you should define it as: "void *found = NULL;" and not as
> acpi_handle anymore.

Oops, you are right of course. That being said, that could (should?)
already be the case with Heiner's patch.

> >  
> > -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > -	if (!vendor || strcmp(vendor, "Dell Inc."))
> > +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> >  		return false;
> > -
> >  	/*
> >  	 * Check that ACPI device SMO88xx is present and is functioning.
> >  	 * Function acpi_get_devices() already filters all ACPI devices
> > @@ -1219,9 +1216,7 @@ static bool is_dell_system_with_lis3lv02
> >  	 * accelerometer but unfortunately ACPI does not provide any other
> >  	 * information (like I2C address).
> >  	 */
> > -	found = false;
> > -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > -			 (void **)&found);
> > +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
> >  
> >  	return found;
> >  }
> > 
> > Basically, it's Heiner's patch except for one line, the idea is to
> > return the HID string pointer (which has type char *) instead of the
> > acpi_handle. That way we don't depend on an opaque ACPI type. (I first
> > tried with the matching acpi_smo8800_ids entry but compiler complained
> > about incompatible pointer types due to the const).
> > 
> > Actually, I think we could use pretty much ANY pointer. Heck, that
> > would work too:
> > 
> > 	*return_value = &disable_features;	/* Could be any address, used as true value */
> > 
> > Would be kinda confusing, but the comment is supposed to address that.
> > In fact we could even do:
> > 
> > 	*return_value = &i;	/* Could be any address, used as true value */
> > 
> > That's the address of a local variable on the stack, which will no
> > longer exist by the time we check it, but that's still a non-NULL
> > pointer so it would work :-D Seriously, let's not do that, simply
> > because static code analyzers would possibly complain.  
> 
> Ehm.. :-) Rather do not talk about other _interesting_ options. Somebody
> could listen and come up with this idea as another workaround for
> misusing ACPI functions API...
> 
> Basically using random pointer values or context-valid pointers just as
> true value can cause issues of leaking pointer to context where it is
> not valid anymore. And very smart code analyzers are correct if they
> throw error that in variable is stored dangling pointer.

Fully agreed, I was only making fun of the situation, this last
proposal was absolutely not meant to be taken seriously.

-- 
Jean Delvare
SUSE L3 Support