[PATCH v2] kexec: Introduce vmcoreinfo signature verification
Xunlei Pang
xpang at redhat.com
Sun Mar 19 21:53:33 PDT 2017
On 03/20/2017 at 11:55 AM, Baoquan He wrote:
> On 03/20/17 at 10:39am, Xunlei Pang wrote:
>> On 03/20/2017 at 10:13 AM, Baoquan He wrote:
>>> On 03/17/17 at 12:22pm, Eric W. Biederman wrote:
>>>> Xunlei Pang <xlpang at redhat.com> writes:
>>>>
>>>>> Currently vmcoreinfo data is updated at boot time subsys_initcall(),
>>>>> it has the risk of being modified by some wrong code during system
>>>>> is running.
>>>>>
>>>>> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on,
>>>>> when using "crash" or "makedumpfile"(etc) utility to parse this vmcore,
>>>>> we probably will get "Segmentation fault" or other unexpected/confusing
>>>>> errors.
>>>> If this is a real concern and the previous discussion sounds like it is
>>>> part of what we need to do is move the variable vmcoreinfo_note out
>>>> of the kernel's .bss section. And modify the code to regenerate
>>>> and keep this information in something like the control page.
>>> I guess this is not from a real issue, just from Xunlei's worry. But
>>> Xunlei didn't give a direct answer to this, and Petr's question. Not
>> It's easy to reproduce: write a kernel module to modify part content of
>> vmcoreinfo_data (we surely have many ways to acquire its VA). If it does
>> exist in theory, we will met it sooner or later in real world due to billions
>> of applications.
>>
>> Also there are bugs like this one
>> https://bugzilla.redhat.com/show_bug.cgi?id=1287097
>> Not sure if it is makedumpfile issue or this one, maybe we can't know forever.
> Well, kdump is not all-purpose. If you write code in module to stomp
> page init_level4_pgt is pointing at, you won't get a vmcore.
vmcoreinfo is a large data chunk prepared for kdump not a normal-sized variable, we better protect it.
> And you are saying vmcoreinfo_data, it's a intermediate page, should be
> vmcoreinfo_note. If the wrong code you mentioned didn't change
> vmcoreinfo_note, but other kernel data which need be saved into
> vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you
> re-saved one.
I am not going to touch vmcoreinfo_note, just trying to relocate vmcoreinfo_data into the crash memory,
then use it to update vmcoreinfo_note which is fully overwritten when crash happens just like the cpu
crash note.
Anyway I will send v3 soon, let further discuss it there, thanks!
Regards,
Xunlei
>
>> Regards,
>> Xunlei
>>
>>> very sure if this will impact other implementation. fadump will be
>>> impacted by this or other dump? Maybe yet or maybe not.
>>>
>>> I don't object this strongly, but please at least add code comment to
>>> explain why vmcoreinfo need be saved twice because it does look weird.
>>>
>>>> Definitely something like this needs a page all to itself, and ideally
>>>> far away from any other kernel data structures. I clearly was not
>>>> watching closely the data someone decided to keep this silly thing
>>>> in the kernel's .bss section.
>>>>
>>>>> As vmcoreinfo is the most fundamental information for vmcore, we better
>>>>> double check its correctness. Here we generate a signature(using crc32)
>>>>> after it is saved, then verify it in crash_save_vmcoreinfo() to see if
>>>>> the signature was broken, if so we have to re-save the vmcoreinfo data
>>>>> to get the correct vmcoreinfo for kdump as possible as we can.
>>>> Sigh. We already have a sha256 that is supposed to cover this sort of
>>>> thing. The bug rather is that apparently it isn't covering this data.
>>>> That sounds like what we should be fixing.
>>>>
>>>> Please let's not invent new mechanisms we have to maintain. Let's
>>>> reorganize this so this static data is protected like all other static
>>>> data in the kexec-on-panic path. We have good mechanims and good
>>>> strategies for avoiding and detecting corruption we just need to use
>>>> them.
>>>>
>>>> Eric
>>>>
>>>>
>>>>
>>>>> Signed-off-by: Xunlei Pang <xlpang at redhat.com>
>>>>> ---
>>>>> v1->v2:
>>>>> - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage"
>>>>> uses the information.
>>>>> - Add crc32 verification for vmcoreinfo, re-save when failure.
>>>>>
>>>>> arch/Kconfig | 1 +
>>>>> kernel/kexec_core.c | 43 +++++++++++++++++++++++++++++++++++--------
>>>>> 2 files changed, 36 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/arch/Kconfig b/arch/Kconfig
>>>>> index c4d6833..66eb296 100644
>>>>> --- a/arch/Kconfig
>>>>> +++ b/arch/Kconfig
>>>>> @@ -4,6 +4,7 @@
>>>>>
>>>>> config KEXEC_CORE
>>>>> bool
>>>>> + select CRC32
>>>>>
>>>>> config HAVE_IMA_KEXEC
>>>>> bool
>>>>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>>>>> index bfe62d5..012acbe 100644
>>>>> --- a/kernel/kexec_core.c
>>>>> +++ b/kernel/kexec_core.c
>>>>> @@ -38,6 +38,7 @@
>>>>> #include <linux/syscore_ops.h>
>>>>> #include <linux/compiler.h>
>>>>> #include <linux/hugetlb.h>
>>>>> +#include <linux/crc32.h>
>>>>>
>>>>> #include <asm/page.h>
>>>>> #include <asm/sections.h>
>>>>> @@ -53,9 +54,10 @@
>>>>>
>>>>> /* vmcoreinfo stuff */
>>>>> static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES];
>>>>> -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
>>>>> +static u32 vmcoreinfo_sig;
>>>>> size_t vmcoreinfo_size;
>>>>> size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data);
>>>>> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
>>>>>
>>>>> /* Flag to indicate we are going to kexec a new kernel */
>>>>> bool kexec_in_progress = false;
>>>>> @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void)
>>>>> final_note(buf);
>>>>> }
>>>>>
>>>>> -void crash_save_vmcoreinfo(void)
>>>>> -{
>>>>> - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds());
>>>>> - update_vmcoreinfo_note();
>>>>> -}
>>>>> -
>>>>> void vmcoreinfo_append_str(const char *fmt, ...)
>>>>> {
>>>>> va_list args;
>>>>> @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
>>>>> return __pa_symbol((unsigned long)(char *)&vmcoreinfo_note);
>>>>> }
>>>>>
>>>>> -static int __init crash_save_vmcoreinfo_init(void)
>>>>> +static void do_crash_save_vmcoreinfo_init(void)
>>>>> {
>>>>> VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
>>>>> VMCOREINFO_PAGESIZE(PAGE_SIZE);
>>>>> @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void)
>>>>> #endif
>>>>>
>>>>> arch_crash_save_vmcoreinfo();
>>>>> +}
>>>>> +
>>>>> +static u32 crash_calc_vmcoreinfo_sig(void)
>>>>> +{
>>>>> + return crc32(~0, vmcoreinfo_data, vmcoreinfo_size);
>>>>> +}
>>>>> +
>>>>> +static bool crash_verify_vmcoreinfo(void)
>>>>> +{
>>>>> + if (crash_calc_vmcoreinfo_sig() == vmcoreinfo_sig)
>>>>> + return true;
>>>>> +
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> +void crash_save_vmcoreinfo(void)
>>>>> +{
>>>>> + /* Re-save if verification fails */
>>>>> + if (!crash_verify_vmcoreinfo()) {
>>>>> + vmcoreinfo_size = 0;
>>>>> + do_crash_save_vmcoreinfo_init();
>>>>> + }
>>>>> +
>>>>> + vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds());
>>>>> + update_vmcoreinfo_note();
>>>>> +}
>>>>> +
>>>>> +static int __init crash_save_vmcoreinfo_init(void)
>>>>> +{
>>>>> + do_crash_save_vmcoreinfo_init();
>>>>> + vmcoreinfo_sig = crash_calc_vmcoreinfo_sig();
>>>>> update_vmcoreinfo_note();
>>>>>
>>>>> return 0;
>>
>> _______________________________________________
>> kexec mailing list
>> kexec at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
More information about the kexec
mailing list