On 10/3/22 12:43, Kees Cook wrote:
>> +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.
