linux-kernel - Re: [PATCH] mei: vsc: Fix wrong invocation of ACPI SID method

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zl42vYs40MJiPxl7@kekkonen.localdomain>
Date: Mon, 3 Jun 2024 21:33:49 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
	Wentong Wu <wentong.wu@...el.com>,
	Tomas Winkler <tomas.winkler@...el.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-acpi@...r.kernel.org, Kate Hsuan <hpa@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mei: vsc: Fix wrong invocation of ACPI SID method

Hi Hans,

Thanks for the patch.

On Mon, Jun 03, 2024 at 10:50:50PM +0200, Hans de Goede wrote:
> When using an initializer for a union only one of the union members
> must be initialized. The initializer for the acpi_object union variable
> passed as argument to the SID ACPI method was initializing both
> the type and the integer members of the union.
> 
> Unfortunately rather then complaining about this gcc simply ignores
> the first initializer and only used the second integer.value = 1
> initializer. Leaving type set to 0 which leads to the argument being
> skipped by acpi acpi_ns_evaluate() resulting in:
> 
> ACPI Warning: \_SB.PC00.SPI1.SPFD.CVFD.SID: Insufficient arguments -
> Caller passed 0, method requires 1 (20240322/nsarguments-232)
> 
> Fix this by initializing only the integer struct part of the union
> and initializing both members of the integer struct.
> 
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> ---
> Even though this is a one-liner, figuring out what was actually going
> wrong here took quite a while.

I was wondering this with Wentong, too...!

> ---
>  drivers/misc/mei/vsc-fw-loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/vsc-fw-loader.c b/drivers/misc/mei/vsc-fw-loader.c
> index ffa4ccd96a10..596a9d695dfc 100644
> --- a/drivers/misc/mei/vsc-fw-loader.c
> +++ b/drivers/misc/mei/vsc-fw-loader.c
> @@ -252,7 +252,7 @@ static int vsc_get_sensor_name(struct vsc_fw_loader *fw_loader,
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
>  	union acpi_object obj = {
> -		.type = ACPI_TYPE_INTEGER,
> +		.integer.type = ACPI_TYPE_INTEGER,
>  		.integer.value = 1,

I guess initialising integer.value implies that all integer fields are set
to zero (and so zeroing type set the line above)? Maybe moving setting type
below setting integer.value might do the trick as well? ;-)

It'd be useful to be warned by the compiler in such a case. There are much
less useful warnings out there.

Excellent finding indeed.

Reviewed-by: Sakari Ailus <sakari.ailus@...ux.intel.com>

This could be cc'd to stable, this warning will display for a lot of
systems. I.e. I think:

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@...r.kernel.org # for 6.8 and later

>  	};
>  	struct acpi_object_list arg_list = {

-- 
Kind regards,

Sakari Ailus