* [BUGFIX PATCH -tip] x86: Fix fixmap page order in text_poke() @ 2009-07-01 21:02 Masami Hiramatsu 2009-07-01 21:17 ` Mathieu Desnoyers 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers 0 siblings, 2 replies; 6+ messages in thread From: Masami Hiramatsu @ 2009-07-01 21:02 UTC (permalink / raw) To: Ingo Molnar, lkml Cc: systemtap, DLE, Masami Hiramatsu, Mathieu Desnoyers, Ingo Molnar Since the fixmap pages are assigned higher address to lower, text_poke() has to use it with inverted order (FIX_TEXT_POKE1 to FIX_TEXT_POKE0). Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> Cc: Ingo Molnar <mingo@elte.hu> --- arch/x86/kernel/alternative.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index f576587..4d8b40b 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -527,14 +527,18 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len) } BUG_ON(!pages[0]); local_irq_save(flags); - set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0])); + /* + * Since the fixmaps are assinged from higher address to lower, + * we use FIX_TEXT_POKE1 first, and FIX_TEXT_POKE0 second. + */ + set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[0])); if (pages[1]) - set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1])); - vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0); + set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[1])); + vaddr = (char *)fix_to_virt(FIX_TEXT_POKE1); memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len); - clear_fixmap(FIX_TEXT_POKE0); + clear_fixmap(FIX_TEXT_POKE1); if (pages[1]) - clear_fixmap(FIX_TEXT_POKE1); + clear_fixmap(FIX_TEXT_POKE0); local_flush_tlb(); sync_core(); /* Could also do a CLFLUSH here to speed up CPU recovery; but -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America), Inc. Software Solutions Division e-mail: mhiramat@redhat.com ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [BUGFIX PATCH -tip] x86: Fix fixmap page order in text_poke() 2009-07-01 21:02 [BUGFIX PATCH -tip] x86: Fix fixmap page order in text_poke() Masami Hiramatsu @ 2009-07-01 21:17 ` Mathieu Desnoyers 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers 1 sibling, 0 replies; 6+ messages in thread From: Mathieu Desnoyers @ 2009-07-01 21:17 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Ingo Molnar, lkml, systemtap, DLE * Masami Hiramatsu (mhiramat@redhat.com) wrote: > Since the fixmap pages are assigned higher address to lower, text_poke() > has to use it with inverted order (FIX_TEXT_POKE1 to FIX_TEXT_POKE0). > Hrm, is that only true for x86_32 or also for x86_64 ? Reading arch/x86/include/asm/fixmap.h : * for x86_32: We allocate these special addresses * from the end of virtual memory (0xfffff000) backwards. * Also this lets us do fail-safe vmalloc(), we * can guarantee that these special addresses and * vmalloc()-ed addresses never overlap. Mathieu > Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> > Cc: Ingo Molnar <mingo@elte.hu> > --- > > arch/x86/kernel/alternative.c | 14 +++++++++----- > 1 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index f576587..4d8b40b 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -527,14 +527,18 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len) > } > BUG_ON(!pages[0]); > local_irq_save(flags); > - set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0])); > + /* > + * Since the fixmaps are assinged from higher address to lower, > + * we use FIX_TEXT_POKE1 first, and FIX_TEXT_POKE0 second. > + */ > + set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[0])); > if (pages[1]) > - set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1])); > - vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0); > + set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[1])); > + vaddr = (char *)fix_to_virt(FIX_TEXT_POKE1); > memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len); > - clear_fixmap(FIX_TEXT_POKE0); > + clear_fixmap(FIX_TEXT_POKE1); > if (pages[1]) > - clear_fixmap(FIX_TEXT_POKE1); > + clear_fixmap(FIX_TEXT_POKE0); > local_flush_tlb(); > sync_core(); > /* Could also do a CLFLUSH here to speed up CPU recovery; but > > > -- > Masami Hiramatsu > > Software Engineer > Hitachi Computer Products (America), Inc. > Software Solutions Division > > e-mail: mhiramat@redhat.com -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 2009-07-01 21:02 [BUGFIX PATCH -tip] x86: Fix fixmap page order in text_poke() Masami Hiramatsu 2009-07-01 21:17 ` Mathieu Desnoyers @ 2009-07-01 21:37 ` Mathieu Desnoyers 2009-07-01 21:58 ` Masami Hiramatsu ` (2 more replies) 1 sibling, 3 replies; 6+ messages in thread From: Mathieu Desnoyers @ 2009-07-01 21:37 UTC (permalink / raw) To: Ingo Molnar, Masami Hiramatsu; +Cc: lkml, systemtap, DLE, stable Masami wrote : > Since the fixmap pages are assigned higher address to lower, text_poke() > has to use it with inverted order (FIX_TEXT_POKE1 to FIX_TEXT_POKE0). I prefer to just invert the order of the fixmap declaration. It's simpler and more straightforward. Backward fixmaps seems to be used by both x86 32 and 64. It's a really nasty bug, because it only hurts when instructions to patch are crossing a page boundary. If this happens, the fixmap write accesses will spill on the following fixmap, which may very well crash the system. And this does not crash the system, it could leave illegal instructions in place. Thanks Masami for finding this. It seems to have crept into the 2.6.30-rc series, so this calls for a -stable inclusion. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> CC: Masami Hiramatsu <mhiramat@redhat.com> Cc: Ingo Molnar <mingo@elte.hu> CC: stable@kernel.org --- arch/x86/include/asm/fixmap.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux-2.6-lttng/arch/x86/include/asm/fixmap.h =================================================================== --- linux-2.6-lttng.orig/arch/x86/include/asm/fixmap.h 2009-07-01 16:52:57.000000000 -0400 +++ linux-2.6-lttng/arch/x86/include/asm/fixmap.h 2009-07-01 16:54:52.000000000 -0400 @@ -111,8 +111,8 @@ enum fixed_addresses { #ifdef CONFIG_PARAVIRT FIX_PARAVIRT_BOOTMAP, #endif - FIX_TEXT_POKE0, /* reserve 2 pages for text_poke() */ - FIX_TEXT_POKE1, + FIX_TEXT_POKE1, /* reserve 2 pages for text_poke() */ + FIX_TEXT_POKE0, /* first page is last, because allocation is backward */ __end_of_permanent_fixed_addresses, #ifdef CONFIG_PROVIDE_OHCI1394_DMA_INIT FIX_OHCI1394_BASE, -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers @ 2009-07-01 21:58 ` Masami Hiramatsu 2009-07-03 8:57 ` [tip:x86/urgent] " tip-bot for Mathieu Desnoyers 2009-07-03 12:44 ` tip-bot for Mathieu Desnoyers 2 siblings, 0 replies; 6+ messages in thread From: Masami Hiramatsu @ 2009-07-01 21:58 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Ingo Molnar, lkml, systemtap, DLE, stable Mathieu Desnoyers wrote: > Masami wrote : > >> Since the fixmap pages are assigned higher address to lower, text_poke() >> has to use it with inverted order (FIX_TEXT_POKE1 to FIX_TEXT_POKE0). > > I prefer to just invert the order of the fixmap declaration. It's simpler and > more straightforward. It's ok for me too. > Backward fixmaps seems to be used by both x86 32 and 64. > > It's a really nasty bug, because it only hurts when instructions to patch are > crossing a page boundary. If this happens, the fixmap write accesses > will spill on the following fixmap, which may very well crash the > system. And this does not crash the system, it could leave illegal > instructions in place. Thanks Masami for finding this. > > It seems to have crept into the 2.6.30-rc series, so this calls for a > -stable inclusion. Right, thanks. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> Acked-by: Masami Hiramatsu <mhiramat@redhat.com> > Cc: Ingo Molnar <mingo@elte.hu> > CC: stable@kernel.org > --- > arch/x86/include/asm/fixmap.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: linux-2.6-lttng/arch/x86/include/asm/fixmap.h > =================================================================== > --- linux-2.6-lttng.orig/arch/x86/include/asm/fixmap.h 2009-07-01 16:52:57.000000000 -0400 > +++ linux-2.6-lttng/arch/x86/include/asm/fixmap.h 2009-07-01 16:54:52.000000000 -0400 > @@ -111,8 +111,8 @@ enum fixed_addresses { > #ifdef CONFIG_PARAVIRT > FIX_PARAVIRT_BOOTMAP, > #endif > - FIX_TEXT_POKE0, /* reserve 2 pages for text_poke() */ > - FIX_TEXT_POKE1, > + FIX_TEXT_POKE1, /* reserve 2 pages for text_poke() */ > + FIX_TEXT_POKE0, /* first page is last, because allocation is backward */ > __end_of_permanent_fixed_addresses, > #ifdef CONFIG_PROVIDE_OHCI1394_DMA_INIT > FIX_OHCI1394_BASE, > -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America), Inc. Software Solutions Division e-mail: mhiramat@redhat.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:x86/urgent] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers 2009-07-01 21:58 ` Masami Hiramatsu @ 2009-07-03 8:57 ` tip-bot for Mathieu Desnoyers 2009-07-03 12:44 ` tip-bot for Mathieu Desnoyers 2 siblings, 0 replies; 6+ messages in thread From: tip-bot for Mathieu Desnoyers @ 2009-07-03 8:57 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, mathieu.desnoyers, hpa, mingo, stable, tglx, mhiramat, mingo Commit-ID: 23202d8f10c901836fb272b5bf1f07a3dd3efb9a Gitweb: http://git.kernel.org/tip/23202d8f10c901836fb272b5bf1f07a3dd3efb9a Author: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> AuthorDate: Wed, 1 Jul 2009 17:37:22 -0400 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Fri, 3 Jul 2009 09:08:34 +0200 x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Masami reported: > Since the fixmap pages are assigned higher address to lower, > text_poke() has to use it with inverted order (FIX_TEXT_POKE1 > to FIX_TEXT_POKE0). I prefer to just invert the order of the fixmap declaration. It's simpler and more straightforward. Backward fixmaps seems to be used by both x86 32 and 64. It's really rare but a nasty bug, because it only hurts when instructions to patch are crossing a page boundary. If this happens, the fixmap write accesses will spill on the following fixmap, which may very well crash the system. And this does not crash the system, it could leave illegal instructions in place. Thanks Masami for finding this. It seems to have crept into the 2.6.30-rc series, so this calls for a -stable inclusion. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> Acked-by: Masami Hiramatsu <mhiramat@redhat.com> Cc: <stable@kernel.org> LKML-Reference: <20090701213722.GH19926@Krystal> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- arch/x86/include/asm/fixmap.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h index 3eb0f79..7b2d71d 100644 --- a/arch/x86/include/asm/fixmap.h +++ b/arch/x86/include/asm/fixmap.h @@ -111,8 +111,8 @@ enum fixed_addresses { #ifdef CONFIG_PARAVIRT FIX_PARAVIRT_BOOTMAP, #endif - FIX_TEXT_POKE0, /* reserve 2 pages for text_poke() */ - FIX_TEXT_POKE1, + FIX_TEXT_POKE1, /* reserve 2 pages for text_poke() */ + FIX_TEXT_POKE0, /* first page is last, because allocation is backward */ __end_of_permanent_fixed_addresses, /* * 256 temporary boot-time mappings, used by early_ioremap(), ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [tip:x86/urgent] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers 2009-07-01 21:58 ` Masami Hiramatsu 2009-07-03 8:57 ` [tip:x86/urgent] " tip-bot for Mathieu Desnoyers @ 2009-07-03 12:44 ` tip-bot for Mathieu Desnoyers 2 siblings, 0 replies; 6+ messages in thread From: tip-bot for Mathieu Desnoyers @ 2009-07-03 12:44 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, mathieu.desnoyers, hpa, mingo, stable, tglx, mhiramat, mingo Commit-ID: 12b9d7ccb841805e347fec8f733f368f43ddba40 Gitweb: http://git.kernel.org/tip/12b9d7ccb841805e347fec8f733f368f43ddba40 Author: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> AuthorDate: Wed, 1 Jul 2009 17:37:22 -0400 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Fri, 3 Jul 2009 14:34:09 +0200 x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Masami reported: > Since the fixmap pages are assigned higher address to lower, > text_poke() has to use it with inverted order (FIX_TEXT_POKE1 > to FIX_TEXT_POKE0). I prefer to just invert the order of the fixmap declaration. It's simpler and more straightforward. Backward fixmaps seems to be used by both x86 32 and 64. It's really rare but a nasty bug, because it only hurts when instructions to patch are crossing a page boundary. If this happens, the fixmap write accesses will spill on the following fixmap, which may very well crash the system. And this does not crash the system, it could leave illegal instructions in place. Thanks Masami for finding this. It seems to have crept into the 2.6.30-rc series, so this calls for a -stable inclusion. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> Acked-by: Masami Hiramatsu <mhiramat@redhat.com> Cc: <stable@kernel.org> LKML-Reference: <20090701213722.GH19926@Krystal> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- arch/x86/include/asm/fixmap.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h index 3eb0f79..7b2d71d 100644 --- a/arch/x86/include/asm/fixmap.h +++ b/arch/x86/include/asm/fixmap.h @@ -111,8 +111,8 @@ enum fixed_addresses { #ifdef CONFIG_PARAVIRT FIX_PARAVIRT_BOOTMAP, #endif - FIX_TEXT_POKE0, /* reserve 2 pages for text_poke() */ - FIX_TEXT_POKE1, + FIX_TEXT_POKE1, /* reserve 2 pages for text_poke() */ + FIX_TEXT_POKE0, /* first page is last, because allocation is backward */ __end_of_permanent_fixed_addresses, /* * 256 temporary boot-time mappings, used by early_ioremap(), ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-07-03 12:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-01 21:02 [BUGFIX PATCH -tip] x86: Fix fixmap page order in text_poke() Masami Hiramatsu 2009-07-01 21:17 ` Mathieu Desnoyers 2009-07-01 21:37 ` [BUGFIX PATCH] x86: Fix fixmap page order for FIX_TEXT_POKE0,1 Mathieu Desnoyers 2009-07-01 21:58 ` Masami Hiramatsu 2009-07-03 8:57 ` [tip:x86/urgent] " tip-bot for Mathieu Desnoyers 2009-07-03 12:44 ` tip-bot for Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox