[Bug report] __arch_hweight32/64 x86

[Bug report] __arch_hweight32/64 x86

[Sebastian Sumpf]

Hello kernel developers,

I recently stumbled over a bug in the inline assembly of '__arch_hweight32' and 
'__arch_hweight64'  in 'arch/x86/include/asm/arch_hweight.h' (present in 6.4 as 
well):

! asm (ALTERNATIVE("call __sw_hweight64", "popcntq %1, %0",
! X86_FEATURE_POPCNT)
! 			 : "="REG_OUT (res)
! 			 : REG_IN (w));

The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails the 
'__sw_hweight' C-function is called from inline assembly with rax and rdi as 
intput/output operands. However, the code does not contain a clobber list of any 
callee safe registers that might be touched by the '__sw_height64' C-function. 
Therefore, these registers will not be restored upon function return by the 
compiler. This leads to varying exceptions/bad behavior caused by the thus 
corrupted registers later on.

Kind regards,

Sebastian


-- 
Sebastian Sumpf
Genode Labs

http://www.genode-labs.com · http://genode.org

Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

Re: [Bug report] __arch_hweight32/64 x86

[Borislav Petkov]

Hello,

On Mon, Jun 26, 2023 at 10:49:44PM +0200, Sebastian Sumpf wrote:
> The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails
> the '__sw_hweight' C-function is called from inline assembly with rax and
> rdi as intput/output operands. However, the code does not contain a clobber
> list of any callee safe registers that might be touched by the
> '__sw_height64' C-function.

Which registers are those? Can you be more specific?

> Therefore, these registers will not be restored
> upon function return by the compiler. This leads to varying
> exceptions/bad behavior caused by the thus corrupted registers later
> on.

How do I reproduce what you're observing so that I can take a look?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [Bug report] __arch_hweight32/64 x86

[Sebastian Sumpf]

Hello Borislav,

On 6/26/23 23:06, Borislav Petkov wrote:
> Hello,
> 
> On Mon, Jun 26, 2023 at 10:49:44PM +0200, Sebastian Sumpf wrote:
>> The 'ALTERNATIVE' macro checks for the popcnt feature. In case this fails
>> the '__sw_hweight' C-function is called from inline assembly with rax and
>> rdi as intput/output operands. However, the code does not contain a clobber
>> list of any callee safe registers that might be touched by the
>> '__sw_height64' C-function.

>> Which registers are those? Can you be more specific?

Registers that are free to use by a called C-function are: rcx, rdx, rsi, rdi, 
and r8-r11 for x86_64. See the "System V Application Binary Interface
AMD64 Architecture Processor Supplement" [1] page 21. x86_32 I would have to 
look up.


>> Therefore, these registers will not be restored
>> upon function return by the compiler. This leads to varying
>> exceptions/bad behavior caused by the thus corrupted registers later
>> on.
> 
> How do I reproduce what you're observing so that I can take a look?

This is hard to tell, I would disable the " X86_FEATURE_POPCNT" feature and use 
the ' CONFIG_ARCH_HAS_FAST_MULTIPLIER' option in order to use the multiplier 
implementation in '__sw_hweight64' in 'lib/hweight.c' At least that is what 
triggered it here.

Regards,

Sebastian

[1] http://www.uclibc.org/docs/psABI-x86_64.pdf

-- 
Sebastian Sumpf
Genode Labs

http://www.genode-labs.com · http://genode.org

Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

Re: [Bug report] __arch_hweight32/64 x86

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On 6/26/23 14:42, Sebastian Sumpf wrote:
> 
>>> Therefore, these registers will not be restored
>>> upon function return by the compiler. This leads to varying
>>> exceptions/bad behavior caused by the thus corrupted registers later
>>> on.
>>
>> How do I reproduce what you're observing so that I can take a look?
> 
> This is hard to tell, I would disable the " X86_FEATURE_POPCNT" feature
> and use the ' CONFIG_ARCH_HAS_FAST_MULTIPLIER' option in order to use
> the multiplier implementation in '__sw_hweight64' in 'lib/hweight.c' At
> least that is what triggered it here.

Looks like you'd also have to be using UML:

$ grep hweight lib/Makefile
obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
$ grep -r GENERIC_HWEIGHT arch/x86
arch/x86/um/Kconfig:config GENERIC_HWEIGHT

I'm not even sure that UML needs GENERIC_HWEIGHT.  From a quick glance,
it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
versions later.  UML just never moved over to the arch-specific versions.

I _think_ the attached patch might just fix the problems with the C
version and bring the x86/UML implementation back in line with the rest
of x86.

Thoughts?

[-- Attachment #2: um-hweight.patch --]
[-- Type: text/x-patch, Size: 289 bytes --]

diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
index 186f13268401..76d507860be4 100644
--- a/arch/x86/um/Kconfig
+++ b/arch/x86/um/Kconfig
@@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
 
 config ARCH_REUSE_HOST_VSYSCALL_AREA
 	def_bool !64BIT
-
-config GENERIC_HWEIGHT
-	def_bool y

Re: [Bug report] __arch_hweight32/64 x86

[Borislav Petkov]

On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
> I'm not even sure that UML needs GENERIC_HWEIGHT.  From a quick glance,
> it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
> versions later.  UML just never moved over to the arch-specific versions.

Thanks - that could very well be the explanation.

That bug report made me blink a couple of times since I did take extra
precaution to not clobber regs in arch/x86/lib/hweight.S as this was
part of the whole pain back then with calling a function from asm where
gcc doesn't even know we're calling a function, see:

f5967101e9de ("x86/hweight: Get rid of the special calling convention")

> I _think_ the attached patch might just fix the problems with the C
> version and bring the x86/UML implementation back in line with the rest
> of x86.
> 
> Thoughts?

> diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
> index 186f13268401..76d507860be4 100644
> --- a/arch/x86/um/Kconfig
> +++ b/arch/x86/um/Kconfig
> @@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
>  
>  config ARCH_REUSE_HOST_VSYSCALL_AREA
>  	def_bool !64BIT
> -
> -config GENERIC_HWEIGHT
> -	def_bool y

Yeah, we should do it. UML should not do anything different wrt calling
conventions so it should be able to handle the arch/x86/lib/hweight.S
versions just fine.

Richi?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [Bug report] __arch_hweight32/64 x86

[Sebastian Sumpf]

On 6/27/23 06:25, Borislav Petkov wrote:
> On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
>> I'm not even sure that UML needs GENERIC_HWEIGHT.  From a quick glance,
>> it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
>> versions later.  UML just never moved over to the arch-specific versions.
> 
> Thanks - that could very well be the explanation.
> 
> That bug report made me blink a couple of times since I did take extra
> precaution to not clobber regs in arch/x86/lib/hweight.S as this was
> part of the whole pain back then with calling a function from asm where
> gcc doesn't even know we're calling a function, see:
> 
> f5967101e9de ("x86/hweight: Get rid of the special calling convention")
> 
>> I _think_ the attached patch might just fix the problems with the C
>> version and bring the x86/UML implementation back in line with the rest
>> of x86.
>>
>> Thoughts?
> 
>> diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
>> index 186f13268401..76d507860be4 100644
>> --- a/arch/x86/um/Kconfig
>> +++ b/arch/x86/um/Kconfig
>> @@ -44,6 +44,3 @@ config ARCH_HAS_SC_SIGNALS
>>   
>>   config ARCH_REUSE_HOST_VSYSCALL_AREA
>>   	def_bool !64BIT
>> -
>> -config GENERIC_HWEIGHT
>> -	def_bool y
> 
> Yeah, we should do it. UML should not do anything different wrt calling
> conventions so it should be able to handle the arch/x86/lib/hweight.S
> versions just fine.

Thank you for the quick resolution! It fixes the problem for me and sorry for 
not explaining well enough.

> Richi?
> 

-- 
Sebastian Sumpf
Genode Labs

http://www.genode-labs.com · http://genode.org

Genode Labs GmbH · Amtsgericht Dresden · HRB 28424 · Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

RE: [Bug report] __arch_hweight32/64 x86

[David Laight]

From: Borislav Petkov
> Sent: 27 June 2023 05:26
> 
> On Mon, Jun 26, 2023 at 03:41:27PM -0700, Dave Hansen wrote:
> > I'm not even sure that UML needs GENERIC_HWEIGHT.  From a quick glance,
> > it looks like x86 used to use GENERIC_HWEIGHT, but got arch-specific
> > versions later.  UML just never moved over to the arch-specific versions.
> 
> Thanks - that could very well be the explanation.
> 
> That bug report made me blink a couple of times since I did take extra
> precaution to not clobber regs in arch/x86/lib/hweight.S as this was
> part of the whole pain back then with calling a function from asm where
> gcc doesn't even know we're calling a function, see:

Perhaps the asm called function should use a different global
label than the one in the C file.

Then it wouldn't be as fragile.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)