Re: 9f4835fb96 ("x86/fpu: Tighten validation of user-supplied .."): Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

[Ingo Molnar]

* kernel test robot <fengguang.wu@xxxxxxxxx> 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 <ebiggers@xxxxxxxxxx>
> AuthorDate: Fri Sep 22 10:41:55 2017 -0700
> Commit:     Ingo Molnar <mingo@xxxxxxxxxx>
> 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