| Message ID | 20220516054051.114490-3-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf_prog_pack followup | expand |
On Sun, 2022-05-15 at 22:40 -0700, Song Liu wrote: > +static void text_poke_memset(void *dst, const void *src, size_t len) > +{ > + int c = *(int *)src; It casts away the const unnecessarily. It could be *(const int *)src. > + > + memset(dst, c, len); > +} > +
> On May 17, 2022, at 12:16 PM, Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Sun, 2022-05-15 at 22:40 -0700, Song Liu wrote: >> +static void text_poke_memset(void *dst, const void *src, size_t len) >> +{ >> + int c = *(int *)src; > > It casts away the const unnecessarily. It could be *(const int *)src. I will fix this in the next version. Or we can ask the maintainer to fix it when applying the patches. Thanks, Song > >> + >> + memset(dst, c, len); >> +} >> +
Hi Peter, > On May 15, 2022, at 10:40 PM, Song Liu <song@kernel.org> wrote: > > Introduce a memset like API for text_poke. This will be used to fill the > unused RX memory with illegal instructions. > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Song Liu <song@kernel.org> Could you please share your comments on this? Thanks, Song > --- > arch/x86/include/asm/text-patching.h | 1 + > arch/x86/kernel/alternative.c | 70 ++++++++++++++++++++++++---- > 2 files changed, 61 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h > index d20ab0921480..1cc15528ce29 100644 > --- a/arch/x86/include/asm/text-patching.h > +++ b/arch/x86/include/asm/text-patching.h > @@ -45,6 +45,7 @@ extern void *text_poke(void *addr, const void *opcode, size_t len); > extern void text_poke_sync(void); > extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len); > extern void *text_poke_copy(void *addr, const void *opcode, size_t len); > +extern void *text_poke_set(void *addr, int c, size_t len); > extern int poke_int3_handler(struct pt_regs *regs); > extern void text_poke_bp(void *addr, const void *opcode, size_t len, const void *emulate); > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index d374cb3cf024..732814065389 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -994,7 +994,21 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state) > __ro_after_init struct mm_struct *poking_mm; > __ro_after_init unsigned long poking_addr; > > -static void *__text_poke(void *addr, const void *opcode, size_t len) > +static void text_poke_memcpy(void *dst, const void *src, size_t len) > +{ > + memcpy(dst, src, len); > +} > + > +static void text_poke_memset(void *dst, const void *src, size_t len) > +{ > + int c = *(int *)src; > + > + memset(dst, c, len); > +} > + > +typedef void text_poke_f(void *dst, const void *src, size_t len); > + > +static void *__text_poke(text_poke_f func, void *addr, const void *src, size_t len) > { > bool cross_page_boundary = offset_in_page(addr) + len > PAGE_SIZE; > struct page *pages[2] = {NULL}; > @@ -1059,7 +1073,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len) > prev = use_temporary_mm(poking_mm); > > kasan_disable_current(); > - memcpy((u8 *)poking_addr + offset_in_page(addr), opcode, len); > + func((u8 *)poking_addr + offset_in_page(addr), src, len); > kasan_enable_current(); > > /* > @@ -1087,11 +1101,13 @@ static void *__text_poke(void *addr, const void *opcode, size_t len) > (cross_page_boundary ? 2 : 1) * PAGE_SIZE, > PAGE_SHIFT, false); > > - /* > - * If the text does not match what we just wrote then something is > - * fundamentally screwy; there's nothing we can really do about that. > - */ > - BUG_ON(memcmp(addr, opcode, len)); > + if (func == text_poke_memcpy) { > + /* > + * If the text does not match what we just wrote then something is > + * fundamentally screwy; there's nothing we can really do about that. > + */ > + BUG_ON(memcmp(addr, src, len)); > + } > > local_irq_restore(flags); > pte_unmap_unlock(ptep, ptl); > @@ -1118,7 +1134,7 @@ void *text_poke(void *addr, const void *opcode, size_t len) > { > lockdep_assert_held(&text_mutex); > > - return __text_poke(addr, opcode, len); > + return __text_poke(text_poke_memcpy, addr, opcode, len); > } > > /** > @@ -1137,7 +1153,7 @@ void *text_poke(void *addr, const void *opcode, size_t len) > */ > void *text_poke_kgdb(void *addr, const void *opcode, size_t len) > { > - return __text_poke(addr, opcode, len); > + return __text_poke(text_poke_memcpy, addr, opcode, len); > } > > /** > @@ -1167,7 +1183,41 @@ void *text_poke_copy(void *addr, const void *opcode, size_t len) > > s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched); > > - __text_poke((void *)ptr, opcode + patched, s); > + __text_poke(text_poke_memcpy, (void *)ptr, opcode + patched, s); > + patched += s; > + } > + mutex_unlock(&text_mutex); > + return addr; > +} > + > +/** > + * text_poke_set - memset into (an unused part of) RX memory > + * @addr: address to modify > + * @c: the byte to fill the area with > + * @len: length to copy, could be more than 2x PAGE_SIZE > + * > + * Not safe against concurrent execution; useful for JITs to dump > + * new code blocks into unused regions of RX memory. Can be used in > + * conjunction with synchronize_rcu_tasks() to wait for existing > + * execution to quiesce after having made sure no existing functions > + * pointers are live. > + */ > +void *text_poke_set(void *addr, int c, size_t len) > +{ > + unsigned long start = (unsigned long)addr; > + size_t patched = 0; > + > + if (WARN_ON_ONCE(core_kernel_text(start))) > + return NULL; > + > + mutex_lock(&text_mutex); > + while (patched < len) { > + unsigned long ptr = start + patched; > + size_t s; > + > + s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched); > + > + __text_poke(text_poke_memset, (void *)ptr, (void *)&c, s); > patched += s; > } > mutex_unlock(&text_mutex); > -- > 2.30.2 >
On Wed, May 18, 2022 at 06:58:46AM +0000, Song Liu wrote: > Hi Peter, > > > On May 15, 2022, at 10:40 PM, Song Liu <song@kernel.org> wrote: > > > > Introduce a memset like API for text_poke. This will be used to fill the > > unused RX memory with illegal instructions. > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Song Liu <song@kernel.org> > > Could you please share your comments on this? I wrote it; it must be good! What specifically you want to know?
> On May 18, 2022, at 12:45 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, May 18, 2022 at 06:58:46AM +0000, Song Liu wrote: >> Hi Peter, >> >>> On May 15, 2022, at 10:40 PM, Song Liu <song@kernel.org> wrote: >>> >>> Introduce a memset like API for text_poke. This will be used to fill the >>> unused RX memory with illegal instructions. >>> >>> Suggested-by: Peter Zijlstra <peterz@infradead.org> >>> Signed-off-by: Song Liu <song@kernel.org> >> >> Could you please share your comments on this? > > I wrote it; it must be good! What specifically you want to know? Maybe just your Acked-by or Signed-off-by? Thanks, Song
On Sun, May 15, 2022 at 10:40:48PM -0700, Song Liu wrote: > Introduce a memset like API for text_poke. This will be used to fill the > unused RX memory with illegal instructions. FWIW, you're going to use it to set INT3 (0xCC), that's not an illegal instruction. INTO (0xCE) would be an illegal instruction (in 64bit mode). > + return addr; > +} > + > +/** > + * text_poke_set - memset into (an unused part of) RX memory > + * @addr: address to modify > + * @c: the byte to fill the area with > + * @len: length to copy, could be more than 2x PAGE_SIZE > + * > + * Not safe against concurrent execution; useful for JITs to dump > + * new code blocks into unused regions of RX memory. Can be used in > + * conjunction with synchronize_rcu_tasks() to wait for existing > + * execution to quiesce after having made sure no existing functions > + * pointers are live. That comment suffers from copy-pasta and needs an update because it clearly isn't correct. > + */ Other than that, seems fine. Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> On May 18, 2022, at 10:09 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Sun, May 15, 2022 at 10:40:48PM -0700, Song Liu wrote: >> Introduce a memset like API for text_poke. This will be used to fill the >> unused RX memory with illegal instructions. > > FWIW, you're going to use it to set INT3 (0xCC), that's not an illegal > instruction. INTO (0xCE) would be an illegal instruction (in 64bit > mode). Hmm… we have been using INT3 as illegal/invalid/special instructions in the JIT. I guess they are equally good for this job? > > >> + return addr; >> +} >> + >> +/** >> + * text_poke_set - memset into (an unused part of) RX memory >> + * @addr: address to modify >> + * @c: the byte to fill the area with >> + * @len: length to copy, could be more than 2x PAGE_SIZE >> + * >> + * Not safe against concurrent execution; useful for JITs to dump >> + * new code blocks into unused regions of RX memory. Can be used in >> + * conjunction with synchronize_rcu_tasks() to wait for existing >> + * execution to quiesce after having made sure no existing functions >> + * pointers are live. > > That comment suffers from copy-pasta and needs an update because it > clearly isn't correct. Will fix in the next version. > >> + */ > > Other than that, seems fine. > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> Thanks, Song
On Wed, May 18, 2022 at 06:34:18PM +0000, Song Liu wrote: > > > > On May 18, 2022, at 10:09 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Sun, May 15, 2022 at 10:40:48PM -0700, Song Liu wrote: > >> Introduce a memset like API for text_poke. This will be used to fill the > >> unused RX memory with illegal instructions. > > > > FWIW, you're going to use it to set INT3 (0xCC), that's not an illegal > > instruction. INTO (0xCE) would be an illegal instruction (in 64bit > > mode). > > Hmm… we have been using INT3 as illegal/invalid/special instructions in > the JIT. I guess they are equally good for this job? INT3 is right, it's just not illegal. Terminology is everything :-) INT3 is the breakpoint instruction, it raises #BP, an illegal instruction would raise #UD. Different exception vectors and such.
diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h index d20ab0921480..1cc15528ce29 100644 --- a/arch/x86/include/asm/text-patching.h +++ b/arch/x86/include/asm/text-patching.h @@ -45,6 +45,7 @@ extern void *text_poke(void *addr, const void *opcode, size_t len); extern void text_poke_sync(void); extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len); extern void *text_poke_copy(void *addr, const void *opcode, size_t len); +extern void *text_poke_set(void *addr, int c, size_t len); extern int poke_int3_handler(struct pt_regs *regs); extern void text_poke_bp(void *addr, const void *opcode, size_t len, const void *emulate); diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index d374cb3cf024..732814065389 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -994,7 +994,21 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state) __ro_after_init struct mm_struct *poking_mm; __ro_after_init unsigned long poking_addr; -static void *__text_poke(void *addr, const void *opcode, size_t len) +static void text_poke_memcpy(void *dst, const void *src, size_t len) +{ + memcpy(dst, src, len); +} + +static void text_poke_memset(void *dst, const void *src, size_t len) +{ + int c = *(int *)src; + + memset(dst, c, len); +} + +typedef void text_poke_f(void *dst, const void *src, size_t len); + +static void *__text_poke(text_poke_f func, void *addr, const void *src, size_t len) { bool cross_page_boundary = offset_in_page(addr) + len > PAGE_SIZE; struct page *pages[2] = {NULL}; @@ -1059,7 +1073,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len) prev = use_temporary_mm(poking_mm); kasan_disable_current(); - memcpy((u8 *)poking_addr + offset_in_page(addr), opcode, len); + func((u8 *)poking_addr + offset_in_page(addr), src, len); kasan_enable_current(); /* @@ -1087,11 +1101,13 @@ static void *__text_poke(void *addr, const void *opcode, size_t len) (cross_page_boundary ? 2 : 1) * PAGE_SIZE, PAGE_SHIFT, false); - /* - * If the text does not match what we just wrote then something is - * fundamentally screwy; there's nothing we can really do about that. - */ - BUG_ON(memcmp(addr, opcode, len)); + if (func == text_poke_memcpy) { + /* + * If the text does not match what we just wrote then something is + * fundamentally screwy; there's nothing we can really do about that. + */ + BUG_ON(memcmp(addr, src, len)); + } local_irq_restore(flags); pte_unmap_unlock(ptep, ptl); @@ -1118,7 +1134,7 @@ void *text_poke(void *addr, const void *opcode, size_t len) { lockdep_assert_held(&text_mutex); - return __text_poke(addr, opcode, len); + return __text_poke(text_poke_memcpy, addr, opcode, len); } /** @@ -1137,7 +1153,7 @@ void *text_poke(void *addr, const void *opcode, size_t len) */ void *text_poke_kgdb(void *addr, const void *opcode, size_t len) { - return __text_poke(addr, opcode, len); + return __text_poke(text_poke_memcpy, addr, opcode, len); } /** @@ -1167,7 +1183,41 @@ void *text_poke_copy(void *addr, const void *opcode, size_t len) s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched); - __text_poke((void *)ptr, opcode + patched, s); + __text_poke(text_poke_memcpy, (void *)ptr, opcode + patched, s); + patched += s; + } + mutex_unlock(&text_mutex); + return addr; +} + +/** + * text_poke_set - memset into (an unused part of) RX memory + * @addr: address to modify + * @c: the byte to fill the area with + * @len: length to copy, could be more than 2x PAGE_SIZE + * + * Not safe against concurrent execution; useful for JITs to dump + * new code blocks into unused regions of RX memory. Can be used in + * conjunction with synchronize_rcu_tasks() to wait for existing + * execution to quiesce after having made sure no existing functions + * pointers are live. + */ +void *text_poke_set(void *addr, int c, size_t len) +{ + unsigned long start = (unsigned long)addr; + size_t patched = 0; + + if (WARN_ON_ONCE(core_kernel_text(start))) + return NULL; + + mutex_lock(&text_mutex); + while (patched < len) { + unsigned long ptr = start + patched; + size_t s; + + s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched); + + __text_poke(text_poke_memset, (void *)ptr, (void *)&c, s); patched += s; } mutex_unlock(&text_mutex);
Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions. Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Song Liu <song@kernel.org> --- arch/x86/include/asm/text-patching.h | 1 + arch/x86/kernel/alternative.c | 70 ++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 10 deletions(-)