Re: [RFC PATCH] x86/cpu: Fix MSR value truncation issue

Andy Lutomirski
2015-10-30T19:27:04Z

On Fri, Oct 30, 2015 at 12:23 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Oct 30, 2015 at 11:59:39AM -0700, Andy Lutomirski wrote:
>> Is this with or without:
>>
>> commit 47edb65178cb7056c2eea0b6c41a7d8c84547192
>> Author: Andy Lutomirski <luto@kernel.org>
>> Date: Thu Jul 23 12:14:40 2015 -0700
>>
>> x86/asm/msr: Make wrmsrl() a function
>
> with.
>
> I'm playing ontop of 4.3-rc7.
>
>> If that patch is applied, then I think that gcc is just being dumb and
>
> Huh, why?
>
> gcc is actually being smart by completely avoiding the shift to the
> upper bits and puts those directly into %edx and sparse simply says that
> we're truncating some bits.
>
> I actually think that sparse is correct in saying that some bits are
> going to be zeroed out even though we want that here.
>
>> that we should consider tweaking wrmsrl to avoid generating the
>> warning. Maybe change (u32)val to (u32)(val & 0xffffffffull)?
>
> That works.

Want to add that to the patch or make it another patch?

>
>> I don't see why we should uglify the caller when the problem is some
>> combination of gcc and the wrmsrl implementation.
>
> Why uglify?
>
> We're basically making explicit that we write the high 32-bits with the
> SYSCALL and SYSRET CS and SS and we set the low 32-bit explicitly to 0:
>
> wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
>
> versus setting the low 32-bits to zero *implicitly*:
>
> wrmsrl(MSR_STAR, ((u64)__USER32_CS)<<48 | ((u64)__KERNEL_CS)<<32);
>
> due to that shifting to the left filling up the 32-bit with zeroes.

Fair enough. I suppose that this thing is a handful of separate
fields as opposed to being just a number. I'm fine with making both
changes.

--Andy