linux-kernel - Re: [PATCH 1/2] cpupower: fix amd cpu (family

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

Message-ID: <61bffd2d-dd37-d1e7-2b19-96d066e10397@gmail.com>
Date:   Mon, 29 Mar 2021 11:51:18 +0800
From:   xufuhai <xufuhai1992@...il.com>
To:     linux@...inikbrodowski.net, sherry.hurwitz@....com, trenn@...e.com,
        linux-pm@...r.kernel.org, rrichter@....com,
        nathan.fontenot@....com, bp@...e.de, latha@...ux.vnet.ibm.com,
        linux-kernel@...r.kernel.org
Cc:     lishujin@...ishou.com
Subject: Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state
 issue

yeah Shuah~ thanks for your reply

For this issue, not meaning "current CPU frequency" but "boost state support--->Active" during 
"cpupower frequency-info" command as below:

	boost state support:
    		Supported: yes
    		Active: yes/no

I think the state returned from the command for amd cpu (family < 0x17) should be like as below:

	as non-root Active state:
		Active: Error while evaluating Boost Capabilities on CPU xx -- are you root?
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)

I don't wanna see the state returned like below:
	
	as non-root Active state:
		Active: yes
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)
	
I will paste the related code by detailed for showing the issue:
	
	if amd cpu family < 0x17 , will run amd_pci_get_num_boost_states function:
-----------------------------------------------------
/linux/tools/power/cpupower/utils/helper/misc.c
 
	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB) {
		*support = 1;

		/* AMD Family 0x17 does not utilize PCI D18F4 like prior
		 * families and has no fixed discrete boost states but
		 * has Hardware determined variable increments instead.
		 */

		if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
			if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
				if (!(val & CPUPOWER_AMD_CPBDIS))
					*active = 1;
			}
		} else {
			ret = amd_pci_get_num_boost_states(active, states);
			if (ret)
				return ret;
		}
---------------------------------------------------

/linux/tools/power/cpupower/utils/helper/amd.c
amd_pci_get_num_boost_states:
	
	val = pci_read_byte(device, 0x15c);

 	if (val & 3)
 		*active = 1;
 	else
----------------------------------------------------

pci_read_byte will memset val to 0xff if caller has no permission to access to read from pci_dev
but for amd_pci_get_num_boost_states, active state will set 1 meaning "yes". I believe that active
state should return negative value to caller function meaning "have no permission" will showing "
Error while evaluating Boost Capabilities on CPU xx -- are you root?"  

----------------------------------------------------
static inline void
pci_read_data(struct pci_dev *d, void *buf, int pos, int len)
{
  if (pos & (len-1))
    d->access->error("Unaligned read: pos=%02x, len=%d", pos, len);
  if (pos + len <= d->cache_len)
    memcpy(buf, d->cache + pos, len);
  else if (!d->methods->read(d, pos, buf, len))
    memset(buf, 0xff, len);
}

byte
pci_read_byte(struct pci_dev *d, int pos)
{
  byte buf;
  pci_read_data(d, &buf, pos, 1);
  return buf;
}
----------------------------------------------------


在 2021/3/24 下午6:27, xufuhai 写道:
> From: xufuhai <xufuhai@...ishou.com>
> 
> For the old  AMD processor (family < 0x17), cpupower will call the
> amd_pci_get_num_boost_states function, but for the non-root user
> pci_read_byte function (implementation comes from the psutil library),
> val will be set to 0xff, indicating that there is no read function
> callback. At this time, the original logic will set the cpupower turbo
> active state to yes. This is an obvious issue~
> 
> Reproduce procedure:
> 	cpupower frequency-info
> 
> Signed-off-by: xufuhai <xufuhai@...ishou.com>
> Signed-off-by: chenguanqiao <chenguanqiao@...ishou.com>
> Signed-off-by: lishujin <lishujin@...ishou.com>
> ---
>  tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 97f2c857048e..6f9504906afa 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
>  		return -ENODEV;
>  
>  	val = pci_read_byte(device, 0x15c);
> +
> +	/* If val is 0xff, meaning has no permisson to
> +	 * get the boost states, return -1
> +	 */
> +	if (val == 0xff)
> +		return -1;
> +
>  	if (val & 3)
>  		*active = 1;
>  	else
>