linux-kernel - Re: [PATCH] ACPI / sysfs: Restructure get

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1766077.IDCbULq7Lr@aspire.rjw.lan>
Date:   Mon, 11 Mar 2019 23:46:14 +0100
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, clang-built-linux@...glegroups.com,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] ACPI / sysfs: Restructure get_status

On Thursday, March 7, 2019 6:38:02 PM CET Nathan Chancellor wrote:
> When building with -Wsometimes-uninitialized, Clang warns:
> 
> drivers/acpi/sysfs.c:667:13: warning: variable 'result' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> 
> Clang can't determine that all cases are covered by the two separate if
> statements. We could combine then to look like this:
> 
>     int result;
> 
>     if (...) {
>         ...
>     } else if {
>         ...
>     } else {
>         result -EINVAL;
>     }
> 
>     return result;
> 
> However, at that point, we can further simplify this function by only
> using result when absolutely needed and just direct returning the value
> of the function.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/388
> Suggested-by: Nick Desaulniers <ndesaulniers@...gle.com>
> Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> ---
>  drivers/acpi/sysfs.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 41324f0b1bee..6ed785cadad9 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -651,23 +651,18 @@ static void acpi_global_event_handler(u32 event_type, acpi_handle device,
>  static int get_status(u32 index, acpi_event_status *status,
>  		      acpi_handle *handle)
>  {
> -	int result;
> -
> -	if (index >= num_gpes + ACPI_NUM_FIXED_EVENTS)
> -		return -EINVAL;
> -
>  	if (index < num_gpes) {
> -		result = acpi_get_gpe_device(index, handle);
> +		int result = acpi_get_gpe_device(index, handle);
>  		if (result) {
>  			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
>  					"Invalid GPE 0x%x", index));
>  			return result;
>  		}
> -		result = acpi_get_gpe_status(*handle, index, status);
> +		return acpi_get_gpe_status(*handle, index, status);
>  	} else if (index < (num_gpes + ACPI_NUM_FIXED_EVENTS))

In principle, it would suffice to replace the "else if (...)" with "else"
to fix the Clang warning AFAICS, but that's not the only issue with this
function.

> -		result = acpi_get_event_status(index - num_gpes, status);
> +		return acpi_get_event_status(index - num_gpes, status);
>  
> -	return result;
> +	return -EINVAL;
>  }
>  
>  static ssize_t counter_show(struct kobject *kobj,

Namely, it confuses errno with acpi_status and may cause the latter to be
returned to user space in certain situations which should never be done.

So, I'd prefer to apply something like the (untested so far) patch below.

---
 drivers/acpi/sysfs.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -648,26 +648,27 @@ static void acpi_global_event_handler(u3
 	}
 }
 
-static int get_status(u32 index, acpi_event_status *status,
+static int get_status(u32 index, acpi_event_status *ret,
 		      acpi_handle *handle)
 {
-	int result;
+	acpi_status status;
 
 	if (index >= num_gpes + ACPI_NUM_FIXED_EVENTS)
 		return -EINVAL;
 
-	if (index < num_gpes) {
-		result = acpi_get_gpe_device(index, handle);
-		if (result) {
-			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
-					"Invalid GPE 0x%x", index));
-			return result;
-		}
-		result = acpi_get_gpe_status(*handle, index, status);
-	} else if (index < (num_gpes + ACPI_NUM_FIXED_EVENTS))
-		result = acpi_get_event_status(index - num_gpes, status);
+	if (index >= num_gpes)
+		return acpi_get_event_status(index - num_gpes, ret);
 
-	return result;
+	status = acpi_get_gpe_device(index, handle);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND, "Invalid GPE 0x%x", index));
+		return -ENXIO;
+	}
+	status = acpi_get_gpe_status(*handle, index, ret);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
 }
 
 static ssize_t counter_show(struct kobject *kobj,