* kernel test robot <fengguang...@intel.com> wrote:
> Greetings,
>
> 0day kernel testing robot got the below dmesg and the first bad commit is
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/fpu
>
> commit 9f4835fb965d8eea7e608d0cb62c246c804dec90
> Author: Eric Biggers <ebigg...@google.com>
> AuthorDate: Fri Sep 22 10:41:55 2017 -0700
> Commit: Ingo Molnar <mi...@kernel.org>
> CommitDate: Sat Sep 23 11:02:00 2017 +0200
>
> x86/fpu: Tighten validation of user-supplied xstate_header
So unfortunately the crash log was not extracted properly by the bot, so we
only
know the subject line:
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
One possibility would be for this memcpy() in copy_kernel_to_xstate() to cause
the
crash:
memcpy(&hdr, kbuf + offset, size);
where 'size' increased from:
size = sizeof(xfeatures);
which was 8 bytes, to:
size = sizeof(hdr);
which is 64 bytes.
What guarantees that 'kbuf + offset + size-1' is still within the kbuf buffer?
AFAICS 'kbuf' gets validated with fpu_user_xstate_size.
... I might be barking up the wrong tree, but I don't see this guaranteed, at
least not in any obvious way.
In hindsight, I think we need to split up this commit:
x86/fpu: Tighten validation of user-supplied xstate_header
Into at least 5-6 parts (!), as it's way too large and risky.
Here is the split-up I'd suggest:
1)
Introduce the new validate_xstate_header() function - without actually using it.
2)
Change xstateregs_set() to use validate_xstate_header() and change the behavior
of
reserved bits. Since this impacts the ABI we better have this as a standalone,
bisectable patch.
3)
Change sanitize_restored_xstate() to use the new validate_xstate_header().
4)
Change copy_kernel_to_xstate() to introduce the new on-kernel-stack header
copy,
but don't yet update the rest of the code, just initialize 'xfeatures' from the
header copy and leave the rest unchanged.
5)
Fix copy_kernel_to_xstate() to now use the header properly, pass it to
validate_xstate_header() and get rid of the 'xfeatures' local variable, etc.
6)
Also, while this change looks correct but it's unrelated and spurious:
- if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+ if (using_compacted_format()) {
and using_compacted_format() is a stupidly global function that adds overhead
unnecessarily:
int using_compacted_format(void)
{
return boot_cpu_has(X86_FEATURE_XSAVES);
}
It should be a static inline instead.
Thanks,
Ingo
