* Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
2017-12-27 19:48 [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR) Linus Torvalds
@ 2017-12-27 19:58 ` Thomas Gleixner
2017-12-27 23:06 ` [tip:x86/urgent] x86-32: Fix " tip-bot for Linus Torvalds
2017-12-28 22:47 ` [PATCH] x86-32: fix " Eric W. Biederman
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2017-12-27 19:58 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexandru Chirvasitu, Ingo Molnar, Andy Lutomirski, kernel list,
Borislav Petkov, Brian Gerst, Denys Vlasenko, H. Peter Anvin,
Josh Poimboeuf, Peter Zijlstra, Steven Rostedt
On Wed, 27 Dec 2017, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Dec 2017 11:41:30 -0800
> Subject: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
>
> Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned up
> and unified the IDT invalidation that existed in a couple of places. It
> changed no actual real code.
>
> Despite not changing any actual real code, it _did_ change code
> generation: by implementing the common idt_invalidate() function in
> archx86/kernel/idt.c, it made the use of the function in
> arch/x86/kernel/machine_kexec_32.c be a real function call rather than
> an (accidental) inlining of the function.
Duh. I just got around reading that thread. Yes, that was definitely not
intended and the situation before my change was not really obvious ...
> That, in turn, exposed two issues:
>
> - in load_segments(), we had incorrectly reset all the segment
> registers, which then made the stack canary load (which gcc does
> using offset of %gs) cause a trap. Instead of %gs pointing to the
> stack canary, it will be the normal zero-based kernel segment, and
> the stack canary load will take a page fault at address 0x14.
>
> - to make this even harder to debug, we had invalidated the GDT just
> before calling idt_invalidate(), which meant that the fault happened
> with an invalid GDT, which in turn causes a triple fault and
> immediate reboot.
Nice detective work.
> Fix this by
>
> (a) not reloading the special segments in load_segments(). We currently
> don't do any percpu accesses (which would require %fs on x86-32) in
> this area, but there's no reason to think that we might not want to
> do them, and like %gs, it's pointless to break it.
>
> (b) doing idt_invalidate() before invalidating the GDT, to keep things
> at least _slightly_ more debuggable for a bit longer. Without a
> IDT, traps will not work. Without a GDT, traps also will not work,
> but neither will any segment loads etc. So in a very real sense,
> the GDT is even more core than the IDT.
>
> Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> I wrote "Reported-and-tested-by: Alexandru" because while this isn't
> exactly the same patch as anything Alexandru tested, it's pretty close,
> and I'm pretty sure this version will fix his issues too.
>
> I decided to try to just do the minimal changes: the GDT invalidation last
> (because of the debugging) and _only_ removing the resetting of fs/gs
> rather than removing load_segments() entirely.
>
> I think making idt_invalidate() be inline would be a good thing as well,
> and I do think that all those "phys_to_virt(0)" things are garbage, but I
> also think they are independent issues, so I didn't touch any of that.
I've put that on the list of stuff to look at once my actual time waster is
done.
> I'm assuming I'll get this patch back through the x86 tree, and will not
> be applying it to my own git tree unless the x86 people ask me to.
I'll pick it up.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread* [tip:x86/urgent] x86-32: Fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
2017-12-27 19:48 [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR) Linus Torvalds
2017-12-27 19:58 ` Thomas Gleixner
@ 2017-12-27 23:06 ` tip-bot for Linus Torvalds
2017-12-28 22:47 ` [PATCH] x86-32: fix " Eric W. Biederman
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Linus Torvalds @ 2017-12-27 23:06 UTC (permalink / raw)
To: linux-tip-commits
Cc: rostedt, bp, brgerst, torvalds, luto, linux-kernel, hpa,
jpoimboe, mingo, achirvasub, dvlasenk, tglx, peterz
Commit-ID: ac461122c88a10b7d775de2f56467f097c9e627a
Gitweb: https://git.kernel.org/tip/ac461122c88a10b7d775de2f56467f097c9e627a
Author: Linus Torvalds <torvalds@linux-foundation.org>
AuthorDate: Wed, 27 Dec 2017 11:48:50 -0800
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 27 Dec 2017 20:59:41 +0100
x86-32: Fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned up
and unified the IDT invalidation that existed in a couple of places. It
changed no actual real code.
Despite not changing any actual real code, it _did_ change code generation:
by implementing the common idt_invalidate() function in
archx86/kernel/idt.c, it made the use of the function in
arch/x86/kernel/machine_kexec_32.c be a real function call rather than an
(accidental) inlining of the function.
That, in turn, exposed two issues:
- in load_segments(), we had incorrectly reset all the segment
registers, which then made the stack canary load (which gcc does
using offset of %gs) cause a trap. Instead of %gs pointing to the
stack canary, it will be the normal zero-based kernel segment, and
the stack canary load will take a page fault at address 0x14.
- to make this even harder to debug, we had invalidated the GDT just
before calling idt_invalidate(), which meant that the fault happened
with an invalid GDT, which in turn causes a triple fault and
immediate reboot.
Fix this by
(a) not reloading the special segments in load_segments(). We currently
don't do any percpu accesses (which would require %fs on x86-32) in
this area, but there's no reason to think that we might not want to
do them, and like %gs, it's pointless to break it.
(b) doing idt_invalidate() before invalidating the GDT, to keep things
at least _slightly_ more debuggable for a bit longer. Without a
IDT, traps will not work. Without a GDT, traps also will not work,
but neither will any segment loads etc. So in a very real sense,
the GDT is even more core than the IDT.
Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/alpine.LFD.2.21.1712271143180.8572@i7.lan
---
arch/x86/kernel/machine_kexec_32.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
index 00bc751..edfede7 100644
--- a/arch/x86/kernel/machine_kexec_32.c
+++ b/arch/x86/kernel/machine_kexec_32.c
@@ -48,8 +48,6 @@ static void load_segments(void)
"\tmovl $"STR(__KERNEL_DS)",%%eax\n"
"\tmovl %%eax,%%ds\n"
"\tmovl %%eax,%%es\n"
- "\tmovl %%eax,%%fs\n"
- "\tmovl %%eax,%%gs\n"
"\tmovl %%eax,%%ss\n"
: : : "eax", "memory");
#undef STR
@@ -232,8 +230,8 @@ void machine_kexec(struct kimage *image)
* The gdt & idt are now invalid.
* If you want to load them you must set up your own idt & gdt.
*/
- set_gdt(phys_to_virt(0), 0);
idt_invalidate(phys_to_virt(0));
+ set_gdt(phys_to_virt(0), 0);
/* now call it */
image->start = relocate_kernel_ptr((unsigned long)image->head,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
2017-12-27 19:48 [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR) Linus Torvalds
2017-12-27 19:58 ` Thomas Gleixner
2017-12-27 23:06 ` [tip:x86/urgent] x86-32: Fix " tip-bot for Linus Torvalds
@ 2017-12-28 22:47 ` Eric W. Biederman
2017-12-28 23:23 ` hpa
2 siblings, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2017-12-28 22:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexandru Chirvasitu, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, kernel list, Borislav Petkov, Brian Gerst,
Denys Vlasenko, H. Peter Anvin, Josh Poimboeuf, Peter Zijlstra,
Steven Rostedt
Linus Torvalds <torvalds@linux-foundation.org> writes:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Dec 2017 11:41:30 -0800
> Subject: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
>
> Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned up
> and unified the IDT invalidation that existed in a couple of places. It
> changed no actual real code.
>
> Despite not changing any actual real code, it _did_ change code
> generation: by implementing the common idt_invalidate() function in
> archx86/kernel/idt.c, it made the use of the function in
> arch/x86/kernel/machine_kexec_32.c be a real function call rather than
> an (accidental) inlining of the function.
>
> That, in turn, exposed two issues:
>
> - in load_segments(), we had incorrectly reset all the segment
> registers, which then made the stack canary load (which gcc does
> using offset of %gs) cause a trap. Instead of %gs pointing to the
> stack canary, it will be the normal zero-based kernel segment, and
> the stack canary load will take a page fault at address 0x14.
>
> - to make this even harder to debug, we had invalidated the GDT just
> before calling idt_invalidate(), which meant that the fault happened
> with an invalid GDT, which in turn causes a triple fault and
> immediate reboot.
>
> Fix this by
>
> (a) not reloading the special segments in load_segments(). We currently
> don't do any percpu accesses (which would require %fs on x86-32) in
> this area, but there's no reason to think that we might not want to
> do them, and like %gs, it's pointless to break it.
>
> (b) doing idt_invalidate() before invalidating the GDT, to keep things
> at least _slightly_ more debuggable for a bit longer. Without a
> IDT, traps will not work. Without a GDT, traps also will not work,
> but neither will any segment loads etc. So in a very real sense,
> the GDT is even more core than the IDT.
>
> Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> I wrote "Reported-and-tested-by: Alexandru" because while this isn't
> exactly the same patch as anything Alexandru tested, it's pretty close,
> and I'm pretty sure this version will fix his issues too.
>
> I decided to try to just do the minimal changes: the GDT invalidation last
> (because of the debugging) and _only_ removing the resetting of fs/gs
> rather than removing load_segments() entirely.
>
> I think making idt_invalidate() be inline would be a good thing as well,
> and I do think that all those "phys_to_virt(0)" things are garbage, but I
> also think they are independent issues, so I didn't touch any of that.
>
> I'm assuming I'll get this patch back through the x86 tree, and will not
> be applying it to my own git tree unless the x86 people ask me to.
>
> Comments?
There is one significant problem with this patch. It changes the ABI
that kexec provides to the next kernel.
That ABI is that the segments will be set to a well defined value.
That value is flat 32bit segments with a base address of 0.
By removing %fs and %gs from load_segments they are now effectively
random undefined values, to the next kernel.
I don't know if anything actually cares. But if they do they are now
broken. It is easy enough to preserve that invariant I don't see
a point in risking potential breaking and looking to see if we have
actually broken the ABI.
It feels like this is something we should move into assembly rather
than attempting to cater to the changing evironment of C code in the
kernel. Or if not we need a big fat comment be very very careful
this code is special.
Eric
> arch/x86/kernel/machine_kexec_32.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
> index 00bc751c861c..edfede768688 100644
> --- a/arch/x86/kernel/machine_kexec_32.c
> +++ b/arch/x86/kernel/machine_kexec_32.c
> @@ -48,8 +48,6 @@ static void load_segments(void)
> "\tmovl $"STR(__KERNEL_DS)",%%eax\n"
> "\tmovl %%eax,%%ds\n"
> "\tmovl %%eax,%%es\n"
> - "\tmovl %%eax,%%fs\n"
> - "\tmovl %%eax,%%gs\n"
> "\tmovl %%eax,%%ss\n"
> : : : "eax", "memory");
> #undef STR
> @@ -232,8 +230,8 @@ void machine_kexec(struct kimage *image)
> * The gdt & idt are now invalid.
> * If you want to load them you must set up your own idt & gdt.
> */
> - set_gdt(phys_to_virt(0), 0);
> idt_invalidate(phys_to_virt(0));
> + set_gdt(phys_to_virt(0), 0);
>
> /* now call it */
> image->start = relocate_kernel_ptr((unsigned long)image->head,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
2017-12-28 22:47 ` [PATCH] x86-32: fix " Eric W. Biederman
@ 2017-12-28 23:23 ` hpa
2017-12-29 0:30 ` Eric W. Biederman
0 siblings, 1 reply; 6+ messages in thread
From: hpa @ 2017-12-28 23:23 UTC (permalink / raw)
To: ebiederm, Linus Torvalds
Cc: Alexandru Chirvasitu, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, kernel list, Borislav Petkov, Brian Gerst,
Denys Vlasenko, Josh Poimboeuf, Peter Zijlstra, Steven Rostedt
On December 28, 2017 2:47:47 PM PST, ebiederm@xmission.com wrote:
>Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> From: Linus Torvalds <torvalds@linux-foundation.org>
>> Date: Wed, 27 Dec 2017 11:41:30 -0800
>> Subject: [PATCH] x86-32: fix kexec with stack canary
>(CONFIG_CC_STACKPROTECTOR)
>>
>> Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned
>up
>> and unified the IDT invalidation that existed in a couple of places.
>It
>> changed no actual real code.
>>
>> Despite not changing any actual real code, it _did_ change code
>> generation: by implementing the common idt_invalidate() function in
>> archx86/kernel/idt.c, it made the use of the function in
>> arch/x86/kernel/machine_kexec_32.c be a real function call rather
>than
>> an (accidental) inlining of the function.
>>
>> That, in turn, exposed two issues:
>>
>> - in load_segments(), we had incorrectly reset all the segment
>> registers, which then made the stack canary load (which gcc does
>> using offset of %gs) cause a trap. Instead of %gs pointing to the
>> stack canary, it will be the normal zero-based kernel segment, and
>> the stack canary load will take a page fault at address 0x14.
>>
>> - to make this even harder to debug, we had invalidated the GDT just
>> before calling idt_invalidate(), which meant that the fault
>happened
>> with an invalid GDT, which in turn causes a triple fault and
>> immediate reboot.
>>
>> Fix this by
>>
>> (a) not reloading the special segments in load_segments(). We
>currently
>> don't do any percpu accesses (which would require %fs on x86-32)
>in
>> this area, but there's no reason to think that we might not want
>to
>> do them, and like %gs, it's pointless to break it.
>>
>> (b) doing idt_invalidate() before invalidating the GDT, to keep
>things
>> at least _slightly_ more debuggable for a bit longer. Without a
>> IDT, traps will not work. Without a GDT, traps also will not
>work,
>> but neither will any segment loads etc. So in a very real sense,
>> the GDT is even more core than the IDT.
>>
>> Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
>> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Peter Anvin <hpa@zytor.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> ---
>>
>> I wrote "Reported-and-tested-by: Alexandru" because while this isn't
>> exactly the same patch as anything Alexandru tested, it's pretty
>close,
>> and I'm pretty sure this version will fix his issues too.
>>
>> I decided to try to just do the minimal changes: the GDT invalidation
>last
>> (because of the debugging) and _only_ removing the resetting of fs/gs
>
>> rather than removing load_segments() entirely.
>>
>> I think making idt_invalidate() be inline would be a good thing as
>well,
>> and I do think that all those "phys_to_virt(0)" things are garbage,
>but I
>> also think they are independent issues, so I didn't touch any of
>that.
>>
>> I'm assuming I'll get this patch back through the x86 tree, and will
>not
>> be applying it to my own git tree unless the x86 people ask me to.
>>
>> Comments?
>
>There is one significant problem with this patch. It changes the ABI
>that kexec provides to the next kernel.
>
>That ABI is that the segments will be set to a well defined value.
>That value is flat 32bit segments with a base address of 0.
>
>By removing %fs and %gs from load_segments they are now effectively
>random undefined values, to the next kernel.
>
>I don't know if anything actually cares. But if they do they are now
>broken. It is easy enough to preserve that invariant I don't see
>a point in risking potential breaking and looking to see if we have
>actually broken the ABI.
>
>It feels like this is something we should move into assembly rather
>than attempting to cater to the changing evironment of C code in the
>kernel. Or if not we need a big fat comment be very very careful
>this code is special.
>
>Eric
>
>
>> arch/x86/kernel/machine_kexec_32.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/machine_kexec_32.c
>b/arch/x86/kernel/machine_kexec_32.c
>> index 00bc751c861c..edfede768688 100644
>> --- a/arch/x86/kernel/machine_kexec_32.c
>> +++ b/arch/x86/kernel/machine_kexec_32.c
>> @@ -48,8 +48,6 @@ static void load_segments(void)
>> "\tmovl $"STR(__KERNEL_DS)",%%eax\n"
>> "\tmovl %%eax,%%ds\n"
>> "\tmovl %%eax,%%es\n"
>> - "\tmovl %%eax,%%fs\n"
>> - "\tmovl %%eax,%%gs\n"
>> "\tmovl %%eax,%%ss\n"
>> : : : "eax", "memory");
>> #undef STR
>> @@ -232,8 +230,8 @@ void machine_kexec(struct kimage *image)
>> * The gdt & idt are now invalid.
>> * If you want to load them you must set up your own idt & gdt.
>> */
>> - set_gdt(phys_to_virt(0), 0);
>> idt_invalidate(phys_to_virt(0));
>> + set_gdt(phys_to_virt(0), 0);
>>
>> /* now call it */
>> image->start = relocate_kernel_ptr((unsigned long)image->head,
The ABI the kernel requires on entry is also documented, and we should stick to that.
That being said, the bottom line is to just stop putting these kinds of final handovers into C and just hope the compiler (or tracing/debugging developers) doesn't randomly break at some thing.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
2017-12-28 23:23 ` hpa
@ 2017-12-29 0:30 ` Eric W. Biederman
0 siblings, 0 replies; 6+ messages in thread
From: Eric W. Biederman @ 2017-12-29 0:30 UTC (permalink / raw)
To: hpa
Cc: Linus Torvalds, Alexandru Chirvasitu, Ingo Molnar,
Thomas Gleixner, Andy Lutomirski, kernel list, Borislav Petkov,
Brian Gerst, Denys Vlasenko, Josh Poimboeuf, Peter Zijlstra,
Steven Rostedt
hpa@zytor.com writes:
> On December 28, 2017 2:47:47 PM PST, ebiederm@xmission.com wrote:
>>Linus Torvalds <torvalds@linux-foundation.org> writes:
>>
>>> From: Linus Torvalds <torvalds@linux-foundation.org>
>>> Date: Wed, 27 Dec 2017 11:41:30 -0800
>>> Subject: [PATCH] x86-32: fix kexec with stack canary
>>(CONFIG_CC_STACKPROTECTOR)
>>>
>>> Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned
>>up
>>> and unified the IDT invalidation that existed in a couple of places.
>>It
>>> changed no actual real code.
>>>
>>> Despite not changing any actual real code, it _did_ change code
>>> generation: by implementing the common idt_invalidate() function in
>>> archx86/kernel/idt.c, it made the use of the function in
>>> arch/x86/kernel/machine_kexec_32.c be a real function call rather
>>than
>>> an (accidental) inlining of the function.
>>>
>>> That, in turn, exposed two issues:
>>>
>>> - in load_segments(), we had incorrectly reset all the segment
>>> registers, which then made the stack canary load (which gcc does
>>> using offset of %gs) cause a trap. Instead of %gs pointing to the
>>> stack canary, it will be the normal zero-based kernel segment, and
>>> the stack canary load will take a page fault at address 0x14.
>>>
>>> - to make this even harder to debug, we had invalidated the GDT just
>>> before calling idt_invalidate(), which meant that the fault
>>happened
>>> with an invalid GDT, which in turn causes a triple fault and
>>> immediate reboot.
>>>
>>> Fix this by
>>>
>>> (a) not reloading the special segments in load_segments(). We
>>currently
>>> don't do any percpu accesses (which would require %fs on x86-32)
>>in
>>> this area, but there's no reason to think that we might not want
>>to
>>> do them, and like %gs, it's pointless to break it.
>>>
>>> (b) doing idt_invalidate() before invalidating the GDT, to keep
>>things
>>> at least _slightly_ more debuggable for a bit longer. Without a
>>> IDT, traps will not work. Without a GDT, traps also will not
>>work,
>>> but neither will any segment loads etc. So in a very real sense,
>>> the GDT is even more core than the IDT.
>>>
>>> Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
>>> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Andy Lutomirski <luto@kernel.org>
>>> Cc: Peter Anvin <hpa@zytor.com>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>> ---
>>>
>>> I wrote "Reported-and-tested-by: Alexandru" because while this isn't
>>> exactly the same patch as anything Alexandru tested, it's pretty
>>close,
>>> and I'm pretty sure this version will fix his issues too.
>>>
>>> I decided to try to just do the minimal changes: the GDT invalidation
>>last
>>> (because of the debugging) and _only_ removing the resetting of fs/gs
>>
>>> rather than removing load_segments() entirely.
>>>
>>> I think making idt_invalidate() be inline would be a good thing as
>>well,
>>> and I do think that all those "phys_to_virt(0)" things are garbage,
>>but I
>>> also think they are independent issues, so I didn't touch any of
>>that.
>>>
>>> I'm assuming I'll get this patch back through the x86 tree, and will
>>not
>>> be applying it to my own git tree unless the x86 people ask me to.
>>>
>>> Comments?
>>
>>There is one significant problem with this patch. It changes the ABI
>>that kexec provides to the next kernel.
>>
>>That ABI is that the segments will be set to a well defined value.
>>That value is flat 32bit segments with a base address of 0.
>>
>>By removing %fs and %gs from load_segments they are now effectively
>>random undefined values, to the next kernel.
>>
>>I don't know if anything actually cares. But if they do they are now
>>broken. It is easy enough to preserve that invariant I don't see
>>a point in risking potential breaking and looking to see if we have
>>actually broken the ABI.
>>
>>It feels like this is something we should move into assembly rather
>>than attempting to cater to the changing evironment of C code in the
>>kernel. Or if not we need a big fat comment be very very careful
>>this code is special.
>>
>>Eric
>>
>>
>>> arch/x86/kernel/machine_kexec_32.c | 4 +---
>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/machine_kexec_32.c
>>b/arch/x86/kernel/machine_kexec_32.c
>>> index 00bc751c861c..edfede768688 100644
>>> --- a/arch/x86/kernel/machine_kexec_32.c
>>> +++ b/arch/x86/kernel/machine_kexec_32.c
>>> @@ -48,8 +48,6 @@ static void load_segments(void)
>>> "\tmovl $"STR(__KERNEL_DS)",%%eax\n"
>>> "\tmovl %%eax,%%ds\n"
>>> "\tmovl %%eax,%%es\n"
>>> - "\tmovl %%eax,%%fs\n"
>>> - "\tmovl %%eax,%%gs\n"
>>> "\tmovl %%eax,%%ss\n"
>>> : : : "eax", "memory");
>>> #undef STR
>>> @@ -232,8 +230,8 @@ void machine_kexec(struct kimage *image)
>>> * The gdt & idt are now invalid.
>>> * If you want to load them you must set up your own idt & gdt.
>>> */
>>> - set_gdt(phys_to_virt(0), 0);
>>> idt_invalidate(phys_to_virt(0));
>>> + set_gdt(phys_to_virt(0), 0);
>>>
>>> /* now call it */
>>> image->start = relocate_kernel_ptr((unsigned long)image->head,
>
> The ABI the kernel requires on entry is also documented, and we should
> stick to that.
Wrong interface this, does not transfer directly to a linux kernel. This
transfers to a shim that starts a linux kernel or something else.
It is way past time to be having design discussions about what this
interface should do. It is more than a decade old.
> That being said, the bottom line is to just stop putting these kinds
> of final handovers into C and just hope the compiler (or
> tracing/debugging developers) doesn't randomly break at some thing.
In general I agree. That makes the code a little less approachable, but
it would seem to remove the chance of surprise interactions even more.
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread