linux-kernel - RE: [PATCH V3 1/2] ACPI / EC: Fix broken big-endian 64bit platforms using 'global

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]

Date:	Wed, 23 Sep 2015 07:52:12 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Viresh Kumar <viresh.kumar@...aro.org>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	"sboyd@...eaurora.org" <sboyd@...eaurora.org>,
	"arnd@...db.de" <arnd@...db.de>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>,
	Len Brown <lenb@...nel.org>,
	"open list:ACPI" <linux-acpi@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V3 1/2] ACPI / EC: Fix broken big-endian 64bit platforms
 using 'global_lock'

Hi,

> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Wednesday, September 16, 2015 9:57 AM
> 
> On Tuesday, September 15, 2015 02:04:58 PM Viresh Kumar wrote:
> > global_lock is defined as an unsigned long and accessing only its lower
> > 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> > for big endian 64 bit systems.
> >
> > Fix that by making global_lock an u32 instead.
> >
> > Cc: <stable@...r.kernel.org>  # v4.1+
> > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> >
> > ---
> > Its marked just for # v4.1+, because arm64 has the first 64 big-endian
> > platform with ACPI. And ACPI support for that is mainlined recently
> > only (Arnd Bergmann).
> 
> OK
> 
> So are you aware of any ARM platform implementing the ACPI EC?
> 
> I am not.  Moreover, I'm not aware of any plans in that area.
> 
> > Another thing worth noticing is that, global_lock is getting an unsigned
> > long long value assigned to it in ec_parse_device() and this is what
> > Arnd had to say about that:
> >
> > "I think that's fine, it does this because the _GLP variable in ACPI is
> > defined as an u64. And that's what happens on 32-bit architectures
> > anyway."
> >
> > This patch should go via GregKH, as the second patch has dependency on
> > it.
> >
> > V2->V3:
> > - Moved this out in a separate patch, so that it can be backported.
> 
> Lv, can you have a look at this, please?
> 
> > ---
> >  drivers/acpi/ec_sys.c   | 2 +-
> >  drivers/acpi/internal.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> > index b4c216bab22b..bea8e425a8de 100644
> > --- a/drivers/acpi/ec_sys.c
> > +++ b/drivers/acpi/ec_sys.c
> > @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
> >  	if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> >  		goto error;
> >  	if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> > -				 (u32 *)&first_ec->global_lock))
> > +				 &first_ec->global_lock))
> >  		goto error;
> >
> >  	if (write_support)
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 9e426210c2a8..9db196de003c 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -138,7 +138,7 @@ struct acpi_ec {
> >  	unsigned long gpe;
> >  	unsigned long command_addr;
> >  	unsigned long data_addr;
> > -	unsigned long global_lock;
> > +	u32 global_lock;
> >  	unsigned long flags;
> >  	unsigned long reference_count;
> >  	struct mutex mutex;
> >
> 

If we really want to change, we may change everything here.
gpe/command_addr/data_addr/global_lock.
And IMO, if we really want to change global_lock, we should make it bool here.

Thanks and best regards
-Lv

> Thanks,
> Rafael