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: <3133462.vhohJv2aGS@aspire.rjw.lan>
Date:   Mon, 11 Mar 2019 23:56:27 +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 Monday, March 11, 2019 11:46:14 PM CET Rafael J. Wysocki wrote:
> 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.

Actually, acpi_get_event_status() returns acpi_status too, so something like
this rather:

---
 drivers/acpi/sysfs.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -648,26 +648,29 @@ 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) {
+		status = acpi_get_gpe_device(index, handle);
+		if (ACPI_FAILURE(status)) {
 			ACPI_EXCEPTION((AE_INFO, AE_NOT_FOUND,
 					"Invalid GPE 0x%x", index));
-			return result;
+			return -ENXIO;
 		}
-		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);
+		status = acpi_get_gpe_status(*handle, index, ret);
+	} else {
+		status = acpi_get_event_status(index - num_gpes, ret);
+	}
+	if (ACPI_FAILURE(status))
+		return -EIO;
 
-	return result;
+	return 0;
 }
 
 static ssize_t counter_show(struct kobject *kobj,