Re: [PATCH v10 05/10] powerpc/bpf: Write protect JIT code
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2021-03-31 10:37:44
Jordan Niethe [off-list ref] writes:
quoted hunk ↗ jump to hunk
Once CONFIG_STRICT_MODULE_RWX is enabled there will be no need to override bpf_jit_free() because it is now possible to set images read-only. So use the default implementation. Also add the necessary call to bpf_jit_binary_lock_ro() which will remove write protection and add exec protection to the JIT image after it has finished being written. Signed-off-by: Jordan Niethe <redacted> --- v10: New to series --- arch/powerpc/net/bpf_jit_comp.c | 5 ++++- arch/powerpc/net/bpf_jit_comp64.c | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-)diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index e809cb5a1631..8015e4a7d2d4 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c@@ -659,12 +659,15 @@ void bpf_jit_compile(struct bpf_prog *fp) bpf_jit_dump(flen, proglen, pass, code_base); bpf_flush_icache(code_base, code_base + (proglen/4)); - #ifdef CONFIG_PPC64 /* Function descriptor nastiness: Address + TOC */ ((u64 *)image)[0] = (u64)code_base; ((u64 *)image)[1] = local_paca->kernel_toc; #endif + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) { + set_memory_ro((unsigned long)image, alloclen >> PAGE_SHIFT); + set_memory_x((unsigned long)image, alloclen >> PAGE_SHIFT); + }
You don't need to check the ifdef in a caller, there are stubs that compile to nothing when CONFIG_ARCH_HAS_SET_MEMORY=n.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c index aaf1a887f653..1484ad588685 100644 --- a/arch/powerpc/net/bpf_jit_comp64.c +++ b/arch/powerpc/net/bpf_jit_comp64.c@@ -1240,6 +1240,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) fp->jited_len = alloclen; bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE)); + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) + bpf_jit_binary_lock_ro(bpf_hdr);
Do we need the ifdef here either? Looks like it should be safe to call due to the stubs.
quoted hunk ↗ jump to hunk
@@ -1262,6 +1264,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) } /* Overriding bpf_jit_free() as we don't set images read-only. */ +#ifndef CONFIG_STRICT_MODULE_RWX
Did you test without this and notice something broken? Looking at the generic version I can't tell why we need to override this. Maybe we don't (anymore?) ? cheers
quoted hunk ↗ jump to hunk
void bpf_jit_free(struct bpf_prog *fp) { unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;@@ -1272,3 +1275,4 @@ void bpf_jit_free(struct bpf_prog *fp) bpf_prog_unlock_free(fp); } +#endif-- 2.25.1