linux-kernel - Re: [PATCH -next] platform/surface: remove status assignment without reading

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <25642c19-6598-42c0-36ad-a2b2d507183a@gmail.com>
Date:   Thu, 29 Oct 2020 14:05:31 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Zou Wei <zou_wei@...wei.com>, hdegoede@...hat.com,
        mgross@...ux.intel.com
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>
Subject: Re: [PATCH -next] platform/surface: remove status assignment without
 reading

On 10/29/20 12:47 PM, Zou Wei wrote:
> The status local variable is assigned but never read:

[...]

>   static int s3_wmi_query_block(const char *guid, int instance, int *ret)
>   {
>   	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> -	acpi_status status;
>   	union acpi_object *obj;
>   	int error = 0;
>   
>   	mutex_lock(&s3_wmi_lock);
> -	status = wmi_query_block(guid, instance, &output);

I assume you want to remove the "status =" and not the whole line...

Regardless, I personally would prefer explicit error handling (as is
done in other contexts with wmi_query_block(), see e.g.
platform/x86/intel-wmi-sbl-fw-update.c. Although, yes, the "if (!obj |
..." should cover the error case as far as I can tell.

Also there seems to be the same issue in platform/x86/msi-wmi.c.

On a related note:

Does anyone know if the mutex here is really required? If there are any
problems, I believe it should rather go to s3_wmi_send_lid_state() and
also wrap the input_report_switch() / input_sync(). Otherwise, if there
were any race-type situations, not covering that could lead to wrong
updates:

     Thread 1                            Thread 2

     s3_query_lid() returns old
                                         s3_query_lid() returns new
                                         input_report_switch()
                                         input_sync()
     input_report_switch()
     input_sync()

I don't really expect those situations here as s3_wmi_send_lid_state()
is only used in probe(), resume(), and notify() (please correct me if
I'm wrong), so the mutex does seem weird to me here.

Otherwise if it's needed, a comment explaining why wouldn't hurt.

Regards,
Max