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
