* [PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher @ 2022-09-23 21:18 Song Liu 2022-09-23 21:18 ` [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher Song Liu 2022-09-23 21:18 ` [PATCH bpf-next 2/2] bpf: Enforce W^X for bpf trampoline Song Liu 0 siblings, 2 replies; 8+ messages in thread From: Song Liu @ 2022-09-23 21:18 UTC (permalink / raw) To: bpf Cc: ast, daniel, john.fastabend, kpsingh, kernel-team, haoluo, jlayton, Song Liu Jeff Layton reported CPA W^X warning linux-next [1]. It turns out to be W^X issue with bpf trampoline and bpf dispatcher. Fix these by: 1. Use bpf_prog_pack for bpf_dispatcher; 2. Set memory permission properly with bpf trampoline. [1] https://lore.kernel.org/lkml/c84cc27c1a5031a003039748c3c099732a718aec.camel@kernel.org/ Song Liu (2): bpf: use bpf_prog_pack for bpf_dispatcher bpf: Enforce W^X for bpf trampoline include/linux/bpf.h | 2 +- include/linux/filter.h | 5 +++++ kernel/bpf/core.c | 9 +++++++-- kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- kernel/bpf/trampoline.c | 22 +++++----------------- 5 files changed, 36 insertions(+), 23 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-23 21:18 [PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher Song Liu @ 2022-09-23 21:18 ` Song Liu 2022-09-23 22:00 ` Alexei Starovoitov 2022-09-23 21:18 ` [PATCH bpf-next 2/2] bpf: Enforce W^X for bpf trampoline Song Liu 1 sibling, 1 reply; 8+ messages in thread From: Song Liu @ 2022-09-23 21:18 UTC (permalink / raw) To: bpf Cc: ast, daniel, john.fastabend, kpsingh, kernel-team, haoluo, jlayton, Song Liu Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher can share pages with bpf programs. This also fixes CPA W^X warnning like: CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... Signed-off-by: Song Liu <song@kernel.org> --- include/linux/bpf.h | 1 + include/linux/filter.h | 5 +++++ kernel/bpf/core.c | 9 +++++++-- kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index edd43edb27d6..a8d0cfe14372 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -946,6 +946,7 @@ struct bpf_dispatcher { struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; int num_progs; void *image; + void *rw_image; u32 image_off; struct bpf_ksym ksym; }; diff --git a/include/linux/filter.h b/include/linux/filter.h index 98e28126c24b..efc42a6e3aed 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); + struct bpf_binary_header * bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, unsigned int alignment, @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); struct bpf_binary_header * bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); +void bpf_prog_pack_free(struct bpf_binary_header *hdr); + static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) { return list_empty(&fp->aux->ksym.lnode) || diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index d1be78c28619..711fd293b6de 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -825,6 +825,11 @@ struct bpf_prog_pack { unsigned long bitmap[]; }; +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) +{ + memset(area, 0, size); +} + #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) static DEFINE_MUTEX(pack_mutex); @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins return pack; } -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) { unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); struct bpf_prog_pack *pack; @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn return ptr; } -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) +void bpf_prog_pack_free(struct bpf_binary_header *hdr) { struct bpf_prog_pack *pack = NULL, *tmp; unsigned int nbits; diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c index 2444bd15cc2d..8a10300854b6 100644 --- a/kernel/bpf/dispatcher.c +++ b/kernel/bpf/dispatcher.c @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) { - void *old, *new; + void *old, *new, *tmp; u32 noff; int err; @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) } new = d->num_progs ? d->image + noff : NULL; + tmp = d->num_progs ? d->rw_image + noff : NULL; if (new) { - if (bpf_dispatcher_prepare(d, new)) + /* Prepare the dispatcher in d->rw_image. Then use + * bpf_arch_text_copy to update d->image, which is RO+X. + */ + if (bpf_dispatcher_prepare(d, tmp)) + return; + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) return; } @@ -140,9 +146,18 @@ void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from, mutex_lock(&d->mutex); if (!d->image) { - d->image = bpf_jit_alloc_exec_page(); + d->image = bpf_prog_pack_alloc(PAGE_SIZE, bpf_jit_fill_hole_with_zero); if (!d->image) goto out; + d->rw_image = bpf_jit_alloc_exec(PAGE_SIZE); + if (!d->rw_image) { + u32 size = PAGE_SIZE; + + bpf_arch_text_copy(d->image, &size, sizeof(size)); + bpf_prog_pack_free((struct bpf_binary_header *)d->image); + d->image = NULL; + goto out; + } bpf_image_ksym_add(d->image, &d->ksym); } -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-23 21:18 ` [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher Song Liu @ 2022-09-23 22:00 ` Alexei Starovoitov 2022-09-23 23:18 ` Song Liu 0 siblings, 1 reply; 8+ messages in thread From: Alexei Starovoitov @ 2022-09-23 22:00 UTC (permalink / raw) To: Song Liu Cc: bpf, Alexei Starovoitov, Daniel Borkmann, John Fastabend, KP Singh, Kernel Team, Hao Luo, jlayton On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote: > > Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher > can share pages with bpf programs. > > This also fixes CPA W^X warnning like: > > CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... > > Signed-off-by: Song Liu <song@kernel.org> > --- > include/linux/bpf.h | 1 + > include/linux/filter.h | 5 +++++ > kernel/bpf/core.c | 9 +++++++-- > kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- > 4 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index edd43edb27d6..a8d0cfe14372 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -946,6 +946,7 @@ struct bpf_dispatcher { > struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; > int num_progs; > void *image; > + void *rw_image; > u32 image_off; > struct bpf_ksym ksym; > }; > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 98e28126c24b..efc42a6e3aed 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; > > typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); > > +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); > + > struct bpf_binary_header * > bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, > unsigned int alignment, > @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); > struct bpf_binary_header * > bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); > > +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); > +void bpf_prog_pack_free(struct bpf_binary_header *hdr); > + > static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) > { > return list_empty(&fp->aux->ksym.lnode) || > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index d1be78c28619..711fd293b6de 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -825,6 +825,11 @@ struct bpf_prog_pack { > unsigned long bitmap[]; > }; > > +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) > +{ > + memset(area, 0, size); > +} > + > #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) > > static DEFINE_MUTEX(pack_mutex); > @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins > return pack; > } > > -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > { > unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); > struct bpf_prog_pack *pack; > @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn > return ptr; > } > > -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) > +void bpf_prog_pack_free(struct bpf_binary_header *hdr) > { > struct bpf_prog_pack *pack = NULL, *tmp; > unsigned int nbits; > diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c > index 2444bd15cc2d..8a10300854b6 100644 > --- a/kernel/bpf/dispatcher.c > +++ b/kernel/bpf/dispatcher.c > @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) > > static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > { > - void *old, *new; > + void *old, *new, *tmp; > u32 noff; > int err; > > @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > } > > new = d->num_progs ? d->image + noff : NULL; > + tmp = d->num_progs ? d->rw_image + noff : NULL; > if (new) { > - if (bpf_dispatcher_prepare(d, new)) > + /* Prepare the dispatcher in d->rw_image. Then use > + * bpf_arch_text_copy to update d->image, which is RO+X. > + */ > + if (bpf_dispatcher_prepare(d, tmp)) > + return; > + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) I don't think we can create a dispatcher with one ip and then copy over into a different location. See emit_bpf_dispatcher() -> emit_cond_near_jump() It's a relative offset jump. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-23 22:00 ` Alexei Starovoitov @ 2022-09-23 23:18 ` Song Liu 2022-09-23 23:23 ` Alexei Starovoitov 0 siblings, 1 reply; 8+ messages in thread From: Song Liu @ 2022-09-23 23:18 UTC (permalink / raw) To: Alexei Starovoitov, Björn Töpel Cc: Song Liu, bpf, Alexei Starovoitov, Daniel Borkmann, John Fastabend, KP Singh, Kernel Team, Hao Luo, jlayton@kernel.org + Björn Töpel > On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote: >> >> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher >> can share pages with bpf programs. >> >> This also fixes CPA W^X warnning like: >> >> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... >> >> Signed-off-by: Song Liu <song@kernel.org> >> --- >> include/linux/bpf.h | 1 + >> include/linux/filter.h | 5 +++++ >> kernel/bpf/core.c | 9 +++++++-- >> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- >> 4 files changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index edd43edb27d6..a8d0cfe14372 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -946,6 +946,7 @@ struct bpf_dispatcher { >> struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; >> int num_progs; >> void *image; >> + void *rw_image; >> u32 image_off; >> struct bpf_ksym ksym; >> }; >> diff --git a/include/linux/filter.h b/include/linux/filter.h >> index 98e28126c24b..efc42a6e3aed 100644 >> --- a/include/linux/filter.h >> +++ b/include/linux/filter.h >> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; >> >> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); >> >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); >> + >> struct bpf_binary_header * >> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, >> unsigned int alignment, >> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); >> struct bpf_binary_header * >> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); >> >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr); >> + >> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) >> { >> return list_empty(&fp->aux->ksym.lnode) || >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c >> index d1be78c28619..711fd293b6de 100644 >> --- a/kernel/bpf/core.c >> +++ b/kernel/bpf/core.c >> @@ -825,6 +825,11 @@ struct bpf_prog_pack { >> unsigned long bitmap[]; >> }; >> >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) >> +{ >> + memset(area, 0, size); >> +} >> + >> #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) >> >> static DEFINE_MUTEX(pack_mutex); >> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins >> return pack; >> } >> >> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) >> { >> unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); >> struct bpf_prog_pack *pack; >> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn >> return ptr; >> } >> >> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr) >> { >> struct bpf_prog_pack *pack = NULL, *tmp; >> unsigned int nbits; >> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c >> index 2444bd15cc2d..8a10300854b6 100644 >> --- a/kernel/bpf/dispatcher.c >> +++ b/kernel/bpf/dispatcher.c >> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) >> >> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) >> { >> - void *old, *new; >> + void *old, *new, *tmp; >> u32 noff; >> int err; >> >> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) >> } >> >> new = d->num_progs ? d->image + noff : NULL; >> + tmp = d->num_progs ? d->rw_image + noff : NULL; >> if (new) { >> - if (bpf_dispatcher_prepare(d, new)) >> + /* Prepare the dispatcher in d->rw_image. Then use >> + * bpf_arch_text_copy to update d->image, which is RO+X. >> + */ >> + if (bpf_dispatcher_prepare(d, tmp)) >> + return; >> + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) > > I don't think we can create a dispatcher with one ip > and then copy over into a different location. > See emit_bpf_dispatcher() -> emit_cond_near_jump() > It's a relative offset jump. Hmm... Yeah, this makes sense. But somehow vmtest doesn't show any issue with this. Is there a better way to test this? Thanks, Song ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-23 23:18 ` Song Liu @ 2022-09-23 23:23 ` Alexei Starovoitov 2022-09-24 0:51 ` Song Liu 0 siblings, 1 reply; 8+ messages in thread From: Alexei Starovoitov @ 2022-09-23 23:23 UTC (permalink / raw) To: Song Liu Cc: Björn Töpel, Song Liu, bpf, Alexei Starovoitov, Daniel Borkmann, John Fastabend, KP Singh, Kernel Team, Hao Luo, jlayton@kernel.org On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote: > > + Björn Töpel > > > On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote: > >> > >> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher > >> can share pages with bpf programs. > >> > >> This also fixes CPA W^X warnning like: > >> > >> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... > >> > >> Signed-off-by: Song Liu <song@kernel.org> > >> --- > >> include/linux/bpf.h | 1 + > >> include/linux/filter.h | 5 +++++ > >> kernel/bpf/core.c | 9 +++++++-- > >> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- > >> 4 files changed, 31 insertions(+), 5 deletions(-) > >> > >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > >> index edd43edb27d6..a8d0cfe14372 100644 > >> --- a/include/linux/bpf.h > >> +++ b/include/linux/bpf.h > >> @@ -946,6 +946,7 @@ struct bpf_dispatcher { > >> struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; > >> int num_progs; > >> void *image; > >> + void *rw_image; > >> u32 image_off; > >> struct bpf_ksym ksym; > >> }; > >> diff --git a/include/linux/filter.h b/include/linux/filter.h > >> index 98e28126c24b..efc42a6e3aed 100644 > >> --- a/include/linux/filter.h > >> +++ b/include/linux/filter.h > >> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; > >> > >> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); > >> > >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); > >> + > >> struct bpf_binary_header * > >> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, > >> unsigned int alignment, > >> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); > >> struct bpf_binary_header * > >> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); > >> > >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); > >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr); > >> + > >> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) > >> { > >> return list_empty(&fp->aux->ksym.lnode) || > >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > >> index d1be78c28619..711fd293b6de 100644 > >> --- a/kernel/bpf/core.c > >> +++ b/kernel/bpf/core.c > >> @@ -825,6 +825,11 @@ struct bpf_prog_pack { > >> unsigned long bitmap[]; > >> }; > >> > >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) > >> +{ > >> + memset(area, 0, size); > >> +} > >> + > >> #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) > >> > >> static DEFINE_MUTEX(pack_mutex); > >> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins > >> return pack; > >> } > >> > >> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > >> { > >> unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); > >> struct bpf_prog_pack *pack; > >> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn > >> return ptr; > >> } > >> > >> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) > >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr) > >> { > >> struct bpf_prog_pack *pack = NULL, *tmp; > >> unsigned int nbits; > >> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c > >> index 2444bd15cc2d..8a10300854b6 100644 > >> --- a/kernel/bpf/dispatcher.c > >> +++ b/kernel/bpf/dispatcher.c > >> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) > >> > >> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > >> { > >> - void *old, *new; > >> + void *old, *new, *tmp; > >> u32 noff; > >> int err; > >> > >> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > >> } > >> > >> new = d->num_progs ? d->image + noff : NULL; > >> + tmp = d->num_progs ? d->rw_image + noff : NULL; > >> if (new) { > >> - if (bpf_dispatcher_prepare(d, new)) > >> + /* Prepare the dispatcher in d->rw_image. Then use > >> + * bpf_arch_text_copy to update d->image, which is RO+X. > >> + */ > >> + if (bpf_dispatcher_prepare(d, tmp)) > >> + return; > >> + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) > > > > I don't think we can create a dispatcher with one ip > > and then copy over into a different location. > > See emit_bpf_dispatcher() -> emit_cond_near_jump() > > It's a relative offset jump. > > Hmm... Yeah, this makes sense. But somehow vmtest doesn't > show any issue with this. Is there a better way to test this? test_xdp*.sh should surely trigger it, but I'm surprised the regular test_run doesn't trigger it. We call bpf_prog_run_xdp() there. We've added if (repeat > 1) bpf_prog_change_xdp(NULL, prog); there to reduce test_progs time. Maybe it reduced test coverage too much. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-23 23:23 ` Alexei Starovoitov @ 2022-09-24 0:51 ` Song Liu 2022-09-24 1:03 ` Alexei Starovoitov 0 siblings, 1 reply; 8+ messages in thread From: Song Liu @ 2022-09-24 0:51 UTC (permalink / raw) To: Alexei Starovoitov Cc: Björn Töpel, Song Liu, bpf, Alexei Starovoitov, Daniel Borkmann, John Fastabend, KP Singh, Kernel Team, Hao Luo, jlayton@kernel.org > On Sep 23, 2022, at 4:23 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote: >> >> + Björn Töpel >> >>> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: >>> >>> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote: >>>> >>>> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher >>>> can share pages with bpf programs. >>>> >>>> This also fixes CPA W^X warnning like: >>>> >>>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... >>>> >>>> Signed-off-by: Song Liu <song@kernel.org> >>>> --- >>>> include/linux/bpf.h | 1 + >>>> include/linux/filter.h | 5 +++++ >>>> kernel/bpf/core.c | 9 +++++++-- >>>> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- >>>> 4 files changed, 31 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >>>> index edd43edb27d6..a8d0cfe14372 100644 >>>> --- a/include/linux/bpf.h >>>> +++ b/include/linux/bpf.h >>>> @@ -946,6 +946,7 @@ struct bpf_dispatcher { >>>> struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; >>>> int num_progs; >>>> void *image; >>>> + void *rw_image; >>>> u32 image_off; >>>> struct bpf_ksym ksym; >>>> }; >>>> diff --git a/include/linux/filter.h b/include/linux/filter.h >>>> index 98e28126c24b..efc42a6e3aed 100644 >>>> --- a/include/linux/filter.h >>>> +++ b/include/linux/filter.h >>>> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; >>>> >>>> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); >>>> >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); >>>> + >>>> struct bpf_binary_header * >>>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, >>>> unsigned int alignment, >>>> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); >>>> struct bpf_binary_header * >>>> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); >>>> >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr); >>>> + >>>> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) >>>> { >>>> return list_empty(&fp->aux->ksym.lnode) || >>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c >>>> index d1be78c28619..711fd293b6de 100644 >>>> --- a/kernel/bpf/core.c >>>> +++ b/kernel/bpf/core.c >>>> @@ -825,6 +825,11 @@ struct bpf_prog_pack { >>>> unsigned long bitmap[]; >>>> }; >>>> >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) >>>> +{ >>>> + memset(area, 0, size); >>>> +} >>>> + >>>> #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) >>>> >>>> static DEFINE_MUTEX(pack_mutex); >>>> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins >>>> return pack; >>>> } >>>> >>>> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) >>>> { >>>> unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); >>>> struct bpf_prog_pack *pack; >>>> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn >>>> return ptr; >>>> } >>>> >>>> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr) >>>> { >>>> struct bpf_prog_pack *pack = NULL, *tmp; >>>> unsigned int nbits; >>>> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c >>>> index 2444bd15cc2d..8a10300854b6 100644 >>>> --- a/kernel/bpf/dispatcher.c >>>> +++ b/kernel/bpf/dispatcher.c >>>> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) >>>> >>>> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) >>>> { >>>> - void *old, *new; >>>> + void *old, *new, *tmp; >>>> u32 noff; >>>> int err; >>>> >>>> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) >>>> } >>>> >>>> new = d->num_progs ? d->image + noff : NULL; >>>> + tmp = d->num_progs ? d->rw_image + noff : NULL; >>>> if (new) { >>>> - if (bpf_dispatcher_prepare(d, new)) >>>> + /* Prepare the dispatcher in d->rw_image. Then use >>>> + * bpf_arch_text_copy to update d->image, which is RO+X. >>>> + */ >>>> + if (bpf_dispatcher_prepare(d, tmp)) >>>> + return; >>>> + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) >>> >>> I don't think we can create a dispatcher with one ip >>> and then copy over into a different location. >>> See emit_bpf_dispatcher() -> emit_cond_near_jump() >>> It's a relative offset jump. >> >> Hmm... Yeah, this makes sense. But somehow vmtest doesn't >> show any issue with this. Is there a better way to test this? > > test_xdp*.sh should surely trigger it, text_xdp*.sh seem to give same result w/ and w/o the set (on top of bpf-next). For example, ./test_xdp_redirect.sh works just fine. (And I think it shouldn't.) > but I'm surprised the regular test_run doesn't trigger it. > We call bpf_prog_run_xdp() there. > We've added > if (repeat > 1) > bpf_prog_change_xdp(NULL, prog); I removed this from test_run.c, but that didn't change vmtest. Song > > there to reduce test_progs time. Maybe it reduced test coverage too much. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher 2022-09-24 0:51 ` Song Liu @ 2022-09-24 1:03 ` Alexei Starovoitov 0 siblings, 0 replies; 8+ messages in thread From: Alexei Starovoitov @ 2022-09-24 1:03 UTC (permalink / raw) To: Song Liu Cc: Björn Töpel, Song Liu, bpf, Alexei Starovoitov, Daniel Borkmann, John Fastabend, KP Singh, Kernel Team, Hao Luo, jlayton@kernel.org On Fri, Sep 23, 2022 at 5:51 PM Song Liu <songliubraving@fb.com> wrote: > > > > > On Sep 23, 2022, at 4:23 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote: > >> > >> + Björn Töpel > >> > >>> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > >>> > >>> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote: > >>>> > >>>> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher > >>>> can share pages with bpf programs. > >>>> > >>>> This also fixes CPA W^X warnning like: > >>>> > >>>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... > >>>> > >>>> Signed-off-by: Song Liu <song@kernel.org> > >>>> --- > >>>> include/linux/bpf.h | 1 + > >>>> include/linux/filter.h | 5 +++++ > >>>> kernel/bpf/core.c | 9 +++++++-- > >>>> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++--- > >>>> 4 files changed, 31 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > >>>> index edd43edb27d6..a8d0cfe14372 100644 > >>>> --- a/include/linux/bpf.h > >>>> +++ b/include/linux/bpf.h > >>>> @@ -946,6 +946,7 @@ struct bpf_dispatcher { > >>>> struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX]; > >>>> int num_progs; > >>>> void *image; > >>>> + void *rw_image; > >>>> u32 image_off; > >>>> struct bpf_ksym ksym; > >>>> }; > >>>> diff --git a/include/linux/filter.h b/include/linux/filter.h > >>>> index 98e28126c24b..efc42a6e3aed 100644 > >>>> --- a/include/linux/filter.h > >>>> +++ b/include/linux/filter.h > >>>> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max; > >>>> > >>>> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size); > >>>> > >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size); > >>>> + > >>>> struct bpf_binary_header * > >>>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr, > >>>> unsigned int alignment, > >>>> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp); > >>>> struct bpf_binary_header * > >>>> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); > >>>> > >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns); > >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr); > >>>> + > >>>> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) > >>>> { > >>>> return list_empty(&fp->aux->ksym.lnode) || > >>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > >>>> index d1be78c28619..711fd293b6de 100644 > >>>> --- a/kernel/bpf/core.c > >>>> +++ b/kernel/bpf/core.c > >>>> @@ -825,6 +825,11 @@ struct bpf_prog_pack { > >>>> unsigned long bitmap[]; > >>>> }; > >>>> > >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size) > >>>> +{ > >>>> + memset(area, 0, size); > >>>> +} > >>>> + > >>>> #define BPF_PROG_SIZE_TO_NBITS(size) (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE) > >>>> > >>>> static DEFINE_MUTEX(pack_mutex); > >>>> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins > >>>> return pack; > >>>> } > >>>> > >>>> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > >>>> { > >>>> unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size); > >>>> struct bpf_prog_pack *pack; > >>>> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn > >>>> return ptr; > >>>> } > >>>> > >>>> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr) > >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr) > >>>> { > >>>> struct bpf_prog_pack *pack = NULL, *tmp; > >>>> unsigned int nbits; > >>>> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c > >>>> index 2444bd15cc2d..8a10300854b6 100644 > >>>> --- a/kernel/bpf/dispatcher.c > >>>> +++ b/kernel/bpf/dispatcher.c > >>>> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image) > >>>> > >>>> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > >>>> { > >>>> - void *old, *new; > >>>> + void *old, *new, *tmp; > >>>> u32 noff; > >>>> int err; > >>>> > >>>> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs) > >>>> } > >>>> > >>>> new = d->num_progs ? d->image + noff : NULL; > >>>> + tmp = d->num_progs ? d->rw_image + noff : NULL; > >>>> if (new) { > >>>> - if (bpf_dispatcher_prepare(d, new)) > >>>> + /* Prepare the dispatcher in d->rw_image. Then use > >>>> + * bpf_arch_text_copy to update d->image, which is RO+X. > >>>> + */ > >>>> + if (bpf_dispatcher_prepare(d, tmp)) > >>>> + return; > >>>> + if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2))) > >>> > >>> I don't think we can create a dispatcher with one ip > >>> and then copy over into a different location. > >>> See emit_bpf_dispatcher() -> emit_cond_near_jump() > >>> It's a relative offset jump. > >> > >> Hmm... Yeah, this makes sense. But somehow vmtest doesn't > >> show any issue with this. Is there a better way to test this? > > > > test_xdp*.sh should surely trigger it, > > text_xdp*.sh seem to give same result w/ and w/o the set (on top > of bpf-next). For example, ./test_xdp_redirect.sh works just fine. > (And I think it shouldn't.) > > > > but I'm surprised the regular test_run doesn't trigger it. > > We call bpf_prog_run_xdp() there. > > We've added > > if (repeat > 1) > > bpf_prog_change_xdp(NULL, prog); > > I removed this from test_run.c, but that didn't change vmtest. Something is broken. That relative jump isn't being triggered. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 2/2] bpf: Enforce W^X for bpf trampoline 2022-09-23 21:18 [PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher Song Liu 2022-09-23 21:18 ` [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher Song Liu @ 2022-09-23 21:18 ` Song Liu 1 sibling, 0 replies; 8+ messages in thread From: Song Liu @ 2022-09-23 21:18 UTC (permalink / raw) To: bpf Cc: ast, daniel, john.fastabend, kpsingh, kernel-team, haoluo, jlayton, Song Liu Mark the trampoline as RO+X after arch_prepare_bpf_trampoline, so that the trampoine follows W^X rule strictly. This will turn off warnings like CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ... Also remove bpf_jit_alloc_exec_page(), since it is not used any more. Signed-off-by: Song Liu <song@kernel.org> --- include/linux/bpf.h | 1 - kernel/bpf/trampoline.c | 22 +++++----------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a8d0cfe14372..44a3d6b3c924 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1008,7 +1008,6 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs); void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from, struct bpf_prog *to); /* Called only from JIT-enabled code, so there's no need for stubs. */ -void *bpf_jit_alloc_exec_page(void); void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym); void bpf_image_ksym_del(struct bpf_ksym *ksym); void bpf_ksym_add(struct bpf_ksym *ksym); diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 41b67eb83ab3..6f7b939321d6 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -116,22 +116,6 @@ bool bpf_prog_has_trampoline(const struct bpf_prog *prog) (ptype == BPF_PROG_TYPE_LSM && eatype == BPF_LSM_MAC); } -void *bpf_jit_alloc_exec_page(void) -{ - void *image; - - image = bpf_jit_alloc_exec(PAGE_SIZE); - if (!image) - return NULL; - - set_vm_flush_reset_perms(image); - /* Keep image as writeable. The alternative is to keep flipping ro/rw - * every time new program is attached or detached. - */ - set_memory_x((long)image, 1); - return image; -} - void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym) { ksym->start = (unsigned long) data; @@ -404,9 +388,10 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx) goto out_free_im; err = -ENOMEM; - im->image = image = bpf_jit_alloc_exec_page(); + im->image = image = bpf_jit_alloc_exec(PAGE_SIZE); if (!image) goto out_uncharge; + set_vm_flush_reset_perms(image); err = percpu_ref_init(&im->pcref, __bpf_tramp_image_release, 0, GFP_KERNEL); if (err) @@ -483,6 +468,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut if (err < 0) goto out; + set_memory_ro((long)im->image, 1); + set_memory_x((long)im->image, 1); + WARN_ON(tr->cur_image && tr->selector == 0); WARN_ON(!tr->cur_image && tr->selector); if (tr->cur_image) -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-24 1:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-23 21:18 [PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher Song Liu 2022-09-23 21:18 ` [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher Song Liu 2022-09-23 22:00 ` Alexei Starovoitov 2022-09-23 23:18 ` Song Liu 2022-09-23 23:23 ` Alexei Starovoitov 2022-09-24 0:51 ` Song Liu 2022-09-24 1:03 ` Alexei Starovoitov 2022-09-23 21:18 ` [PATCH bpf-next 2/2] bpf: Enforce W^X for bpf trampoline Song Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox