On Tue, Oct 04, 2022 at 10:17:57AM +0000, David Laight wrote:

From: Dave Hansen

quoted

Sent: 03 October 2022 21:05

On 10/3/22 12:43, Kees Cook wrote:

quoted

quoted

+static inline void set_clr_bits_msrl(u32 msr, u64 set, u64 clear)
+{
+	u64 val, new_val;
+
+	rdmsrl(msr, val);
+	new_val = (val & ~clear) | set;
+
+	if (new_val != val)
+		wrmsrl(msr, new_val);
+}

I always get uncomfortable when I see these kinds of generalized helper
functions for touching cpu bits, etc. It just begs for future attacker
abuse to muck with arbitrary bits -- even marked inline there is a risk
the compiler will ignore that in some circumstances (not as currently
used in the code, but I'm imagining future changes leading to such a
condition). Will you humor me and change this to a macro instead? That'll
force it always inline (even __always_inline isn't always inline):

Oh, are you thinking that this is dangerous because it's so surgical and
non-intrusive?  It's even more powerful to an attacker than, say
wrmsrl(), because there they actually have to know what the existing
value is to update it.  With this helper, it's quite easy to flip an
individual bit without disturbing the neighboring bits.

Is that it?

I don't _like_ the #defines, but doing one here doesn't seem too onerous
considering how critical MSRs are.

How often is the 'msr' number not a compile-time constant?
Adding rd/wrmsr variants that verify this would reduce the
attack surface as well.

Oh, yes! I do this all the time with FORTIFY shenanigans. Right, so,
instead of a macro, the "cannot be un-inlined" could be enforced with
this (untested):

static __always_inline void set_clr_bits_msrl(u32 msr, u64 set, u64 clear)
{
	u64 val, new_val;

	BUILD_BUG_ON(!__builtin_constant_p(msr) ||
		     !__builtin_constant_p(set) ||
		     !__builtin_constant_p(clear));

	rdmsrl(msr, val);
	new_val = (val & ~clear) | set;

	if (new_val != val)
		wrmsrl(msr, new_val);
}

-- 
Kees Cook