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

[Andy Lutomirski]

On Fri, Oct 30, 2015 at 12:23 PM, Borislav Petkov <bp@xxxxxxxxx> 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@xxxxxxxxxx>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/