Messages in this thread | | | From | Andy Lutomirski <> | Date | Fri, 30 Oct 2015 12:26:42 -0700 | Subject | Re: [RFC PATCH] x86/cpu: Fix MSR value truncation issue |
| |
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
| |