linux-kernel - RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit

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:	Tue, 31 May 2016 07:13:47 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	Mike Marshall <hubcap@...ibond.com>
CC:	Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Brown, Len" <len.brown@...el.com>,
	"Moore, Robert" <robert.moore@...el.com>
Subject: RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in
 acpi_hw_get_access_bit_width()

Hi, Boris and Mike

Please help to validate if this version can also fix your issues.
After enumerating the possible cases, I realized that the address check might not be necessary.
But we need a max_bit_width check in this function to make it prepared for a future usage in acpi_read()/acpi_write().
Thanks in advance.

Best regards
-Lv

> From: Zheng, Lv
> Subject: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in
> acpi_hw_get_access_bit_width()
> 
> The address check in acpi_hw_get_access_bit_width() should be byte
> width
> based, not bit width based. This patch fixes this mistake.
> 
> For those who want to review acpi_hw_access_bit_width(), here is the
> concerns and the design details of the function:
> 
> It is supposed that the GAS Address field should be aligned to the byte
> width indicated by the GAS AccessSize field. Similarly, for the old non
> GAS register, it is supposed that its Address should be aligned to its
> Length.
> For the "AccessSize = 0 (meaning ANY)" case, we try to return the
> maximum
> instruction width (64 for MMIO or 32 for PIO) or the user expected access
> bit width (64 for acpi_read()/acpi_write() or 32 for acpi_hw_read()/
> acpi_hw_write()) for futher operation and it is supposed that the GAS
> Address field should always be aligned to the maximum expected access
> bit
> width (otherwise it can't be ANY).
> 
> The problem is in acpi_tb_init_generic_address(), where the non GAS
> register's Length is converted into the GAS BitWidth field, its Address is
> converted into the GAS Address field, and the GAS AccessSize field is left
> 0 but most of the register actually cannot be accessed using "ANY"
> accesses.
> 
> As a conclusion, when AccessSize = 0 (ANY), the Address should either be
> aligned to the BitWidth (wrong conversion) or aligned to 32 (PIO) or 64
> (MMIO). Since BitWidth for the wrong conversion is 8,16,32, the Address
> of the real GAS should always be aligned to 8,16,32, the address alignment
> check is not necessary. But we in fact could enhance the check for a future
> case where max_bit_width could be 64 for a PIO access issued from
> acpi_read()/acpi_write().
> 
> Fixes: b314a172ee96 ("ACPICA: Hardware: Add optimized access bit width
> support")
> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> Cc: Mike Marshall <hubcap@...ibond.com>
> Suggested-by: Jan Beulich <jbeulich@...e.com>
> Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> ---
>  drivers/acpi/acpica/hwregs.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index 0f18dbc..0553c0b 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -86,24 +86,22 @@ acpi_hw_get_access_bit_width(struct
> acpi_generic_address *reg, u8 max_bit_width)
>  	u64 address;
> 
>  	if (!reg->access_width) {
> +		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> +			max_bit_width = 32;
> +		}
>  		/*
>  		 * Detect old register descriptors where only the bit_width
> field
>  		 * makes senses. The target address is copied to handle
> possible
>  		 * alignment issues.
>  		 */
>  		ACPI_MOVE_64_TO_64(&address, &reg->address);
> -		if (!reg->bit_offset && reg->bit_width &&
> +		if (reg->bit_width < max_bit_width &&
> +		    !reg->bit_offset && reg->bit_width &&
>  		    ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
> -		    ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> -		    ACPI_IS_ALIGNED(address, reg->bit_width)) {
> +		    ACPI_IS_ALIGNED(reg->bit_width, 8)) {
>  			return (reg->bit_width);
> -		} else {
> -			if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> {
> -				return (32);
> -			} else {
> -				return (max_bit_width);
> -			}
>  		}
> +		return (max_bit_width);
>  	} else {
>  		return (1 << (reg->access_width + 2));
>  	}
> --
> 1.7.10