linux-kernel - Re: [RFC PATCH] Use feature bit names in clearcpuid=

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: <20200920161628.GA3951950@rani.riverdale.lan>
Date:   Sun, 20 Sep 2020 12:16:28 -0400
From:   Arvind Sankar <nivedita@...m.mit.edu>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] Use feature bit names in clearcpuid=

On Sun, Sep 20, 2020 at 05:42:28PM +0200, Borislav Petkov wrote:
> Hi,
> 
> so tglx hates this clearcpuid= interface where you have to give the
> X86_FEATURE array indices in order to disable a feature bit for testing.
> Below is a first attempt (lightly tested in a VM only) to accept the bit
> names from /proc/cpuinfo too.
> 
> I say "too" because not all feature bits have names and we would still
> have to support the numbers. Yeah, yuck.
> 
> An exemplary cmdline would then be something like:
> 
> clearcpuid=de,440,smca,succory,bmi1,3dnow ("succory" is wrong on
> purpose).
> 
> and it says:
> 
> [    0.000000] Clearing CPUID bits: de 13:24 smca bmi1 3dnow
> 
> Also, I'm thinking we should taint the kernel when this option is used.
> 
> Thoughts?

I like it. Allowing 13:24 as input would be icing on the cake :)

Small comments below.

> @@ -273,21 +273,45 @@ static void __init fpu__init_parse_early_param(void)
>  		return;
>  
>  	pr_info("Clearing CPUID bits:");
> -	do {
> -		res = get_option(&argptr, &bit);
> -		if (res == 0 || res == 3)
> -			break;
> -
> -		/* If the argument was too long, the last bit may be cut off */
> -		if (res == 1 && arglen >= sizeof(arg))
> -			break;
> -
> -		if (bit >= 0 && bit < NCAPINTS * 32) {
> -			pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
> -			setup_clear_cpu_cap(bit);
> +
> +	while (argptr) {
> +		int i;
> +
> +		opt = (strsep(&argptr, ","));
> +		if (!opt)
> +			continue;

The !opt check is unnecessary: strsep() returns NULL iff argptr is NULL
on entry. The parentheses around strsep() also look odd.

> +
> +		if (!kstrtoint(opt, 10, &bit)) {

Could make bit unsigned and use kstrtouint().

> +			if (bit >= 0 && bit < NCAPINTS * 32) {
> +				if (!x86_cap_flag(bit))
> +					pr_cont(" " X86_CAP_FMT_BARE, x86_cap_flag_bare(bit));
> +				else
> +					pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
> +
> +				setup_clear_cpu_cap(bit);
> +				taint++;
> +				continue;
> +			}

Could always continue if it was a number, even if it was invalid, since
that shouldn't match a name in any case?

>  		}
> -	} while (res == 2);
> +
> +#ifdef CONFIG_X86_FEATURE_NAMES
> +		for (i = 0; i < 32 * NCAPINTS; i++) {
> +			if (!x86_cap_flags[i])
> +				continue;
> +
> +			if (strcmp(x86_cap_flags[i], opt))
> +				continue;
> +
> +			pr_cont(" %s", opt);
> +			setup_clear_cpu_cap(i);
> +			taint++;

We could break out of the loop here -- we can't have multiple bits with
the same name, right?

> +		}
> +#endif
> +	}
>  	pr_cont("\n");
> +
> +	if (taint)
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>  }
>  
>  /*
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette