linux-kernel - Re: [PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled

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: <l5cpgzcxhpfbvpdvlaz3deomb5lkpyzgfbklgg6wmcaw3vicga@b6wzenpmg7jm>
Date: Fri, 24 Jan 2025 21:09:28 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: <shuah@...nel.org>, <hpa@...or.com>, <x86@...nel.org>,
	<dave.hansen@...ux.intel.com>, <bp@...en8.de>, <mingo@...hat.com>,
	<tglx@...utronix.de>, <linux-kernel@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>, <kirill@...temov.name>, "Kirill A.
 Shutemov" <kirill.shutemov@...ux.intel.com>, Shuah Khan
	<skhan@...uxfoundation.org>
Subject: Re: [PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled

On 2025-01-24 at 08:23:09 -0800, Dave Hansen wrote:
>On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
>> +static inline int kernel_has_lam(void)
>> +{
>> +	unsigned long bits;
>> +
>> +	syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>> +	return !!bits;
>> +}
>
>Generally, I'm less picky about selftest/ code than in-kernel code. But
>people really do take selftest code and use it as a starting point for
>production code.
>
>I'd much rather have overly verbose, obviously correct code:
>
>	err = syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>
>	/* Handle syscall failure, like pre-LAM kernels: */
>	if (err)
>		return 0
>
>	/* Tag bits are empty on non-LAM systems: */
>	return !!bits;
>

Sure, more comments is always good :)

>Actually, I was going to argue for that^ just on style and writing good
>code. But then I spotted a bug. What happens if the kernel has
>CONFIG_ADDRESS_MASKING=n, either because it is config'd off or it's old?
>The:
>
>	put_user(0, (unsigned long __user *)arg2);
>
>won't ever get run and 'bits' will be uninitialized.

Huh, yeah, you're right. I tested it with both CONFIG_ADDRESS_MASKING=n and =y,
and on systems with it both available and not available but must've been a
coincidence it worked.

I'll fix the checks and init bits for the next version.

>
>So, I think this code was trying to be compact, fast and clever. But it
>really just turns out to be buggy.
>

-- 
Kind regards
Maciej Wieczór-Retman