On Fri, Jan 12, 2018 at 10:58 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Fri, Jan 12, 2018 at 10:21:43AM -0800, Dan Williams wrote:
>> > That just sounds wrong. What if the speculation starts *after* the
>> > access_ok() check? Then the barrier has no purpose.
>> >
>> > Most access_ok/get_user/copy_from_user calls are like this:
>> >
>> > if (copy_from_user(...uptr..)) /* or access_ok() or get_user() */
>> > return -EFAULT;
>> >
>> > So in other words, the usercopy function is called *before* the branch.
>> >
>> > But to halt speculation, the lfence needs to come *after* the branch.
>> > So putting lfences *before* the branch doesn't solve anything.
>> >
>> > So what am I missing?
>>
>> We're trying to prevent a pointer under user control from being
>> de-referenced inside the kernel, before we know it has been limited to
>> something safe. In the following sequence the branch we are worried
>> about speculating is the privilege check:
>>
>> if (access_ok(uptr)) /* <--- Privelege Check */
>> if (copy_from_user_(uptr))
>>
>> The cpu can speculatively skip that access_ok() check and cause a read
>> of kernel memory.
>
> Converting your example code to assembly:
>
> call access_ok # no speculation which started before this point is allowed to continue past this point
> test %rax, %rax
> jne error_path
> dereference_uptr:
> (do nefarious things with the user pointer)
>
> error_path:
> mov -EINVAL, %rax
> ret
>
> So the CPU is still free to speculately execute the dereference_uptr
> block because the lfence was before the 'jne error_path' branch.
By the time we get to de-reference uptr we know it is not pointing at
kernel memory, because access_ok would have failed and the cpu would
have waited for that failure result before doing anything else.
Now the vulnerability that still exists after this fence is if we do
something with the value returned from de-referencing that pointer.
I.e. if we do:
get_user(val, uptr);
if (val < array_max)
array[val];
...then we're back to having a user controlled pointer in the kernel.
This is the point where we need array_ptr() to sanitize things.
