| 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
| ||
|
Message-ID: <a28ddc26-d77a-470a-a33f-88144f717e86@sifive.com>
Date: Thu, 18 Jul 2024 20:55:58 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Alexandre Ghiti <alexghiti@...osinc.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>,
Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
<palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Peter Zijlstra <peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>,
Jason Baron <jbaron@...mai.com>, Steven Rostedt <rostedt@...dmis.org>,
Ard Biesheuvel <ardb@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Björn Töpel
<bjorn@...nel.org>, Pu Lehui <pulehui@...wei.com>,
Puranjay Mohan <puranjay@...nel.org>, Luke Nelson <luke.r.nels@...il.com>,
Xi Wang <xi.wang@...il.com>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH] riscv: patch: Remove redundant functions
Hi Alex,
On 2024-07-17 3:41 AM, Alexandre Ghiti wrote:
> Commit edf2d546bfd6f5c4 ("riscv: patch: Flush the icache right after
> patching to avoid illegal insns") removed the last differences between
> patch_text_set_nosync() and patch_insn_set(), and between
> patch_text_nosync() and patch_insn_write().
>
> So remove the redundant *_nosync() functions.
My understanding was that we would eventually revert that patch, once we are
sure we never non-atomically patch the text patching code. So it's helpful to
keep the semantic distinction between the two sets of functions.
And looking at this closer, I think the original patch should not have removed
the calls to flush_icache_range() anyway. It replaces a global icache flush with
a local icache flush, which is wrong if there is more than one CPU online, and
there are a couple of places (bpf_jit_core.c, kprobes.c) where those functions
are called at runtime.
Regards,
Samuel
> Reported-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> Closes: https://lore.kernel.org/linux-riscv/CAMuHMdUwx=rU2MWhFTE6KhYHm64phxx2Y6u05-aBLGfeG5696A@mail.gmail.com/
> Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> ---
> arch/riscv/errata/sifive/errata.c | 4 ++--
> arch/riscv/errata/thead/errata.c | 2 +-
> arch/riscv/include/asm/patch.h | 3 +--
> arch/riscv/kernel/alternative.c | 4 ++--
> arch/riscv/kernel/cpufeature.c | 2 +-
> arch/riscv/kernel/jump_label.c | 2 +-
> arch/riscv/kernel/patch.c | 24 +-----------------------
> arch/riscv/net/bpf_jit_core.c | 4 ++--
> 8 files changed, 11 insertions(+), 34 deletions(-)
>
> diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
> index 716cfedad3a2..5253b205aa17 100644
> --- a/arch/riscv/errata/sifive/errata.c
> +++ b/arch/riscv/errata/sifive/errata.c
> @@ -112,8 +112,8 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
> tmp = (1U << alt->patch_id);
> if (cpu_req_errata & tmp) {
> mutex_lock(&text_mutex);
> - patch_text_nosync(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
> - alt->alt_len);
> + patch_insn_write(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
> + alt->alt_len);
> mutex_unlock(&text_mutex);
> cpu_apply_errata |= tmp;
> }
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index bf6a0a6318ee..0ce280a190b6 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -182,7 +182,7 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
> memcpy(oldptr, altptr, alt->alt_len);
> } else {
> mutex_lock(&text_mutex);
> - patch_text_nosync(oldptr, altptr, alt->alt_len);
> + patch_insn_write(oldptr, altptr, alt->alt_len);
> mutex_unlock(&text_mutex);
> }
> }
> diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
> index 9f5d6e14c405..6b0e9b8a321b 100644
> --- a/arch/riscv/include/asm/patch.h
> +++ b/arch/riscv/include/asm/patch.h
> @@ -6,9 +6,8 @@
> #ifndef _ASM_RISCV_PATCH_H
> #define _ASM_RISCV_PATCH_H
>
> +int patch_insn_set(void *addr, u8 c, size_t len);
> int patch_insn_write(void *addr, const void *insn, size_t len);
> -int patch_text_nosync(void *addr, const void *insns, size_t len);
> -int patch_text_set_nosync(void *addr, u8 c, size_t len);
> int patch_text(void *addr, u32 *insns, int ninsns);
>
> extern int riscv_patch_in_stop_machine;
> diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
> index 0128b161bfda..a8b508d99cf8 100644
> --- a/arch/riscv/kernel/alternative.c
> +++ b/arch/riscv/kernel/alternative.c
> @@ -83,7 +83,7 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn,
> riscv_insn_insert_utype_itype_imm(&call[0], &call[1], imm);
>
> /* patch the call place again */
> - patch_text_nosync(ptr, call, sizeof(u32) * 2);
> + patch_insn_write(ptr, call, sizeof(u32) * 2);
> }
>
> static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset)
> @@ -98,7 +98,7 @@ static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset)
> riscv_insn_insert_jtype_imm(&jal_insn, imm);
>
> /* patch the call place again */
> - patch_text_nosync(ptr, &jal_insn, sizeof(u32));
> + patch_insn_write(ptr, &jal_insn, sizeof(u32));
> }
>
> void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len,
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5ef48cb20ee1..4c040a857c7e 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -795,7 +795,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
> altptr = ALT_ALT_PTR(alt);
>
> mutex_lock(&text_mutex);
> - patch_text_nosync(oldptr, altptr, alt->alt_len);
> + patch_insn_write(oldptr, altptr, alt->alt_len);
> riscv_alternative_fix_offsets(oldptr, alt->alt_len, oldptr - altptr);
> mutex_unlock(&text_mutex);
> }
> diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> index e6694759dbd0..74b5ebfacf4a 100644
> --- a/arch/riscv/kernel/jump_label.c
> +++ b/arch/riscv/kernel/jump_label.c
> @@ -36,6 +36,6 @@ void arch_jump_label_transform(struct jump_entry *entry,
> }
>
> mutex_lock(&text_mutex);
> - patch_text_nosync(addr, &insn, sizeof(insn));
> + patch_insn_write(addr, &insn, sizeof(insn));
> mutex_unlock(&text_mutex);
> }
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index ab03732d06c4..bf45b507f900 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -177,7 +177,7 @@ static int __patch_insn_write(void *addr, const void *insn, size_t len)
> NOKPROBE_SYMBOL(__patch_insn_write);
> #endif /* CONFIG_MMU */
>
> -static int patch_insn_set(void *addr, u8 c, size_t len)
> +int patch_insn_set(void *addr, u8 c, size_t len)
> {
> size_t patched = 0;
> size_t size;
> @@ -198,17 +198,6 @@ static int patch_insn_set(void *addr, u8 c, size_t len)
> }
> NOKPROBE_SYMBOL(patch_insn_set);
>
> -int patch_text_set_nosync(void *addr, u8 c, size_t len)
> -{
> - u32 *tp = addr;
> - int ret;
> -
> - ret = patch_insn_set(tp, c, len);
> -
> - return ret;
> -}
> -NOKPROBE_SYMBOL(patch_text_set_nosync);
> -
> int patch_insn_write(void *addr, const void *insn, size_t len)
> {
> size_t patched = 0;
> @@ -230,17 +219,6 @@ int patch_insn_write(void *addr, const void *insn, size_t len)
> }
> NOKPROBE_SYMBOL(patch_insn_write);
>
> -int patch_text_nosync(void *addr, const void *insns, size_t len)
> -{
> - u32 *tp = addr;
> - int ret;
> -
> - ret = patch_insn_write(tp, insns, len);
> -
> - return ret;
> -}
> -NOKPROBE_SYMBOL(patch_text_nosync);
> -
> static int patch_text_cb(void *data)
> {
> struct patch_insn *patch = data;
> diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
> index 0a96abdaca65..b053ae5c4191 100644
> --- a/arch/riscv/net/bpf_jit_core.c
> +++ b/arch/riscv/net/bpf_jit_core.c
> @@ -226,7 +226,7 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> int ret;
>
> mutex_lock(&text_mutex);
> - ret = patch_text_nosync(dst, src, len);
> + ret = patch_insn_write(dst, src, len);
> mutex_unlock(&text_mutex);
>
> if (ret)
> @@ -240,7 +240,7 @@ int bpf_arch_text_invalidate(void *dst, size_t len)
> int ret;
>
> mutex_lock(&text_mutex);
> - ret = patch_text_set_nosync(dst, 0, len);
> + ret = patch_insn_set(dst, 0, len);
> mutex_unlock(&text_mutex);
>
> return ret;
Powered by blists - more mailing lists