| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 12 Jun 2015 00:41:18 -0700
From: Andy Lutomirski <luto@...capital.net>
To: Ingo Molnar <mingo@...nel.org>
Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Ingo Molnar <mingo@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
"Rafael J. Wysocki" <rjw@...ysocki.net>, X86 ML <x86@...nel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Denys Vlasenko <dvlasenk@...hat.com>,
Borislav Petkov <bp@...en8.de>,
Brian Gerst <brgerst@...il.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] x86: General protection fault after STR (32 bit systems only)
On Thu, Jun 11, 2015 at 11:07 PM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com> wrote:
>
>> Suspend to RAM process is returning to userspsace with DS set to KERNEL_DS
>> after resume, this cause general protection fault. [...]
>
> But s2ram has no influence on 'returning to user-space'. So unless I'm missing
> something this changelog makes no sense.
>
> Unfortunately the patch seems to be completely bogus as well:
>
>> [...] This is very difficult to reproduce, but this does happen on 32 bit
>> systems. This crash is not reproduced after save/restore DS during calls to
>> _save_processor_state/ __restore_processor_state respectively.
>>
>> Similar issue was reported in the past
>> https://bugzilla.kernel.org/show_bug.cgi?id=61781, for which the root cause
>> was not identified.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
>> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>> ---
>> arch/x86/include/asm/suspend_32.h | 2 +-
>> arch/x86/power/cpu.c | 2 ++
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h
>> index 552d6c9..3503d0b 100644
>> --- a/arch/x86/include/asm/suspend_32.h
>> +++ b/arch/x86/include/asm/suspend_32.h
>> @@ -11,7 +11,7 @@
>>
>> /* image of the saved processor state */
>> struct saved_context {
>> - u16 es, fs, gs, ss;
>> + u16 ds, es, fs, gs, ss;
>> unsigned long cr0, cr2, cr3, cr4;
>> u64 misc_enable;
>> bool misc_enable_saved;
>> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
>> index 757678f..e0dfb01 100644
>> --- a/arch/x86/power/cpu.c
>> +++ b/arch/x86/power/cpu.c
>> @@ -79,6 +79,7 @@ static void __save_processor_state(struct saved_context *ctxt)
>> * segment registers
>> */
>> #ifdef CONFIG_X86_32
>> + savesegment(ds, ctxt->ds);
>> savesegment(es, ctxt->es);
>> savesegment(fs, ctxt->fs);
>> savesegment(gs, ctxt->gs);
>> @@ -198,6 +199,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
>> * segment registers
>> */
>> #ifdef CONFIG_X86_32
>> + loadsegment(ds, ctxt->ds);
>> loadsegment(es, ctxt->es);
>> loadsegment(fs, ctxt->fs);
>> loadsegment(gs, ctxt->gs);
>
> So save_processor_state() is used by 32-bit s2ram in the following place:
>
> arch/x86/kernel/acpi/wakeup_32.S: call save_processor_state
>
> Other uses are:
>
> arch/x86/kernel/acpi/wakeup_64.S: call save_processor_state
> arch/x86/kernel/apm_32.c: save_processor_state();
> arch/x86/kernel/machine_kexec_32.c: save_processor_state();
> arch/x86/kernel/machine_kexec_64.c: save_processor_state();
> arch/x86/platform/olpc/xo1-wakeup.S: call save_processor_state
> kernel/power/hibernate.c: save_processor_state();
> kernel/power/hibernate.c: save_processor_state();
>
> but neither of these call sites seems to matter to the bug: the 32-bit system does
> not use APM, kexec, is not an OLPC and does not use hibernation.
>
> And if we look at arch/x86/kernel/acpi/wakeup_32.S it's a straightforward full
> state save/restore before ACPI (BIOS) downcalls.
>
> Furthermore, the bug report in:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=61781
>
> talks about SIGSEGVs, and claims that this regression triggers in v3.11 but does
> not trigger in v3.10.
>
> 1)
>
> So the first critical question is: if the ACPI/BIOS suspend code corrupts the
> kernel's DS, how can we get so far as to resume fully, return to user-space, and
> segfault there so that it can all be reported?
>
> So neither the explanation nor the code makes any sense in the context of the
> reported bugs. Can anyone else offer any plausible theory about why this patch
> would fix 32-bit user-space segfaults?
>
> 2)
>
> Btw., I don't mind the idea of the patch itself per se: saving/restoring DS is
> prudent to avoid the BIOS stomping on our DS.
>
> But the restoration done in this patch is bogus, it's done way too late in a C
> function, there's a number of places where we might use the kernel DS before it's
> restored, such as restore_registers():
>
> restore_registers:
> movl saved_context_ebp, %ebp
> movl saved_context_ebx, %ebx
> movl saved_context_esi, %esi
> movl saved_context_edi, %edi
> pushl saved_context_eflags
> popfl
> ret
>
> this is called before restore_processor_state().
>
> those 'saved_context_*' references are already using the kernel DS! So if it's
> corrupted, we'll crash there already. So your patch seems totally nonsensical.
>
> 3)
>
> So the patch below (totally untested) does the DS restoration early on, as the
> first thing after we emerge from the sleep. This should be equivalent to your
> patch, but is more robust and much simpler: there's no need to save DS, because we
> know that it has to be __KERNEL_DS.
>
> Could you please test whether this solves the problem as well? Also, could you
> please describe how the failure triggers in your system: how many times do you
> have to suspend/resume to trigger the segfaults, and is there anything that makes
> the failures less or more likely?
>
> Thanks,
>
> Ingo
>
> =================>
> arch/x86/kernel/acpi/wakeup_32.S | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
> index 665c6b7d2ea9..9a3ce66e0dd8 100644
> --- a/arch/x86/kernel/acpi/wakeup_32.S
> +++ b/arch/x86/kernel/acpi/wakeup_32.S
> @@ -81,6 +81,10 @@ ENTRY(do_suspend_lowlevel)
> jmp ret_point
> .p2align 4,,7
> ret_point:
> + /* In case the BIOS corrupted DS, make the kernel context minimally functional: */
> + movl $__KERNEL_DS, %eax
> + movl %eax, %ds
> +
On further thought, I think you want movl $__USER_DS, %eax. The
32-bit kernel is a strange beast. Also, you should probably fix up
%es as well.
--Andy
> call restore_registers
> call restore_processor_state
> ret
--
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists