* Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-02-18 10:55 ` [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
@ 2024-02-18 15:19 ` Kees Cook
2024-02-19 15:06 ` Puranjay Mohan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2024-02-18 15:19 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Russell King, Puranjay Mohan, Zi Shen Lim, Catalin Marinas,
Will Deacon, Tiezhu Yang, Hengqi Chen, Huacai Chen, WANG Xuerui,
Johan Almbladh, Paul Burton, Thomas Bogendoerfer,
James E.J. Bottomley, Helge Deller, Ilya Leoshkevich,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, David S. Miller,
Andreas Larsson, Wang YanQing, David Ahern, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
bpf, linux-arm-kernel, linux-kernel, loongarch, linux-mips,
linux-parisc, linux-s390, sparclinux, netdev,
linux-hardening @ vger . kernel . org
On Sun, Feb 18, 2024 at 11:55:02AM +0100, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
> arch/arm/net/bpf_jit_32.c | 25 ++++++++++++-------------
> arch/arm64/net/bpf_jit_comp.c | 21 +++++++++++++++------
> arch/loongarch/net/bpf_jit.c | 21 +++++++++++++++------
> arch/mips/net/bpf_jit_comp.c | 3 ++-
> arch/parisc/net/bpf_jit_core.c | 8 +++++++-
> arch/s390/net/bpf_jit_comp.c | 6 +++++-
> arch/sparc/net/bpf_jit_comp_64.c | 6 +++++-
> arch/x86/net/bpf_jit_comp32.c | 3 +--
> include/linux/filter.h | 4 ++--
> 9 files changed, 64 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 1d672457d02f..01516f83a95a 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -2222,28 +2222,21 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> /* If building the body of the JITed code fails somehow,
> * we fall back to the interpretation.
> */
> - if (build_body(&ctx) < 0) {
> - image_ptr = NULL;
> - bpf_jit_binary_free(header);
> - prog = orig_prog;
> - goto out_imms;
> - }
> + if (build_body(&ctx) < 0)
> + goto out_free;
> build_epilogue(&ctx);
>
> /* 3.) Extra pass to validate JITed Code */
> - if (validate_code(&ctx)) {
> - image_ptr = NULL;
> - bpf_jit_binary_free(header);
> - prog = orig_prog;
> - goto out_imms;
> - }
> + if (validate_code(&ctx))
> + goto out_free;
> flush_icache_range((u32)header, (u32)(ctx.target + ctx.idx));
>
> if (bpf_jit_enable > 1)
> /* there are 2 passes here */
> bpf_jit_dump(prog->len, image_size, 2, ctx.target);
>
> - bpf_jit_binary_lock_ro(header);
> + if (bpf_jit_binary_lock_ro(header))
> + goto out_free;
> prog->bpf_func = (void *)ctx.target;
> prog->jited = 1;
> prog->jited_len = image_size;
> @@ -2260,5 +2253,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> bpf_jit_prog_release_other(prog, prog == orig_prog ?
> tmp : orig_prog);
> return prog;
> +
> +out_free:
> + image_ptr = NULL;
> + bpf_jit_binary_free(header);
> + prog = orig_prog;
> + goto out_imms;
These gotos give me the creeps, but yes, it does appear to be in the
style of the existing error handling.
> [...]
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index b18ce19981ec..f2be1dcf3b24 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -2600,8 +2600,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> if (bpf_jit_enable > 1)
> bpf_jit_dump(prog->len, proglen, pass + 1, image);
>
> - if (image) {
> - bpf_jit_binary_lock_ro(header);
> + if (image && !bpf_jit_binary_lock_ro(header)) {
I find the "!" kind of hard to read the "inverted" logic (0 is success),
so if this gets a revision, maybe do "== 0"?:
if (image && bpf_jit_binary_lock_ro(header) == 0) {
But that's just me. So, regardless:
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-02-18 10:55 ` [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
2024-02-18 15:19 ` Kees Cook
@ 2024-02-19 15:06 ` Puranjay Mohan
2024-02-19 15:33 ` Ilya Leoshkevich
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Puranjay Mohan @ 2024-02-19 15:06 UTC (permalink / raw)
To: Christophe Leroy, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Russell King, Zi Shen Lim, Catalin Marinas,
Will Deacon, Tiezhu Yang, Hengqi Chen, Huacai Chen, WANG Xuerui,
Johan Almbladh, Paul Burton, Thomas Bogendoerfer,
James E.J. Bottomley, Helge Deller, Ilya Leoshkevich,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, David S. Miller,
Andreas Larsson, Wang YanQing, David Ahern, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin
Cc: Christophe Leroy, bpf, linux-arm-kernel, linux-kernel, loongarch,
linux-mips, linux-parisc, linux-s390, sparclinux, netdev,
Kees Cook, linux-hardening @ vger . kernel . org
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
> arch/arm/net/bpf_jit_32.c | 25 ++++++++++++-------------
> arch/arm64/net/bpf_jit_comp.c | 21 +++++++++++++++------
> arch/loongarch/net/bpf_jit.c | 21 +++++++++++++++------
> arch/mips/net/bpf_jit_comp.c | 3 ++-
> arch/parisc/net/bpf_jit_core.c | 8 +++++++-
> arch/s390/net/bpf_jit_comp.c | 6 +++++-
> arch/sparc/net/bpf_jit_comp_64.c | 6 +++++-
> arch/x86/net/bpf_jit_comp32.c | 3 +--
> include/linux/filter.h | 4 ++--
> 9 files changed, 64 insertions(+), 33 deletions(-)
>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Thanks,
Puranjay Mohan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-02-18 10:55 ` [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
2024-02-18 15:19 ` Kees Cook
2024-02-19 15:06 ` Puranjay Mohan
@ 2024-02-19 15:33 ` Ilya Leoshkevich
2024-02-20 1:22 ` Tiezhu Yang
2024-02-20 8:56 ` Johan Almbladh
4 siblings, 0 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2024-02-19 15:33 UTC (permalink / raw)
To: Christophe Leroy, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Russell King, Puranjay Mohan, Zi Shen Lim,
Catalin Marinas, Will Deacon, Tiezhu Yang, Hengqi Chen,
Huacai Chen, WANG Xuerui, Johan Almbladh, Paul Burton,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, David S. Miller,
Andreas Larsson, Wang YanQing, David Ahern, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin
Cc: bpf, linux-arm-kernel, linux-kernel, loongarch, linux-mips,
linux-parisc, linux-s390, sparclinux, netdev, Kees Cook,
linux-hardening @ vger . kernel . org
On Sun, 2024-02-18 at 11:55 +0100, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it
> modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial
> conflict with unmodified bpf_prog_lock_ro().
> ---
> arch/arm/net/bpf_jit_32.c | 25 ++++++++++++-------------
> arch/arm64/net/bpf_jit_comp.c | 21 +++++++++++++++------
> arch/loongarch/net/bpf_jit.c | 21 +++++++++++++++------
> arch/mips/net/bpf_jit_comp.c | 3 ++-
> arch/parisc/net/bpf_jit_core.c | 8 +++++++-
> arch/s390/net/bpf_jit_comp.c | 6 +++++-
> arch/sparc/net/bpf_jit_comp_64.c | 6 +++++-
> arch/x86/net/bpf_jit_comp32.c | 3 +--
> include/linux/filter.h | 4 ++--
> 9 files changed, 64 insertions(+), 33 deletions(-)
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com> # s390x
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-02-18 10:55 ` [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
` (2 preceding siblings ...)
2024-02-19 15:33 ` Ilya Leoshkevich
@ 2024-02-20 1:22 ` Tiezhu Yang
2024-02-20 8:56 ` Johan Almbladh
4 siblings, 0 replies; 6+ messages in thread
From: Tiezhu Yang @ 2024-02-20 1:22 UTC (permalink / raw)
To: Christophe Leroy, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Russell King, Puranjay Mohan, Zi Shen Lim,
Catalin Marinas, Will Deacon, Hengqi Chen, Huacai Chen,
WANG Xuerui, Johan Almbladh, Paul Burton, Thomas Bogendoerfer,
James E.J. Bottomley, Helge Deller, Ilya Leoshkevich,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, David S. Miller,
Andreas Larsson, Wang YanQing, David Ahern, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin
Cc: bpf, linux-arm-kernel, linux-kernel, loongarch, linux-mips,
linux-parisc, linux-s390, sparclinux, netdev, Kees Cook,
linux-hardening @ vger . kernel . org
On 02/18/2024 06:55 PM, Christophe Leroy wrote:
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
...
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index e73323d759d0..aafc5037fd2b 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1294,16 +1294,18 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> flush_icache_range((unsigned long)header, (unsigned long)(ctx.image + ctx.idx));
>
> if (!prog->is_func || extra_pass) {
> + int err;
> +
> if (extra_pass && ctx.idx != jit_data->ctx.idx) {
> pr_err_once("multi-func JIT bug %d != %d\n",
> ctx.idx, jit_data->ctx.idx);
> - bpf_jit_binary_free(header);
> - prog->bpf_func = NULL;
> - prog->jited = 0;
> - prog->jited_len = 0;
> - goto out_offset;
> + goto out_free;
> + }
> + err = bpf_jit_binary_lock_ro(header);
> + if (err) {
> + pr_err_once("bpf_jit_binary_lock_ro() returned %d\n", err);
> + goto out_free;
> }
> - bpf_jit_binary_lock_ro(header);
> } else {
> jit_data->ctx = ctx;
> jit_data->image = image_ptr;
> @@ -1334,6 +1336,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> out_offset = -1;
>
> return prog;
> +
> +out_free:
> + bpf_jit_binary_free(header);
> + prog->bpf_func = NULL;
> + prog->jited = 0;
> + prog->jited_len = 0;
> + goto out_offset;
> }
>
> /* Indicate the JIT backend supports mixing bpf2bpf and tailcalls. */
...
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index fc0994dc5c72..314414fa6d70 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -892,10 +892,10 @@ static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp)
> return 0;
> }
>
> -static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
> +static inline int __must_check bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
> {
> set_vm_flush_reset_perms(hdr);
> - set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
> + return set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT);
> }
>
> int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap);
LoongArch does not select CONFIG_ARCH_HAS_SET_MEMORY, set_memory_ro()
and set_memory_x() always return 0, then set_memory_rox() also returns
0, that is to say, bpf_jit_binary_lock_ro() will return 0, it seems
that there is no obvious effect for LoongArch with this patch.
But once CONFIG_ARCH_HAS_SET_MEMORY is selected and the arch-specified
set_memory_*() functions are implemented in the future, it is necessary
to handle the error cases. At least, in order to keep consistent with
the other archs, the code itself looks good to me.
Acked-by: Tiezhu Yang <yangtiezhu@loongson.cn> # LoongArch
Thanks,
Tiezhu
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()
2024-02-18 10:55 ` [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro() Christophe Leroy
` (3 preceding siblings ...)
2024-02-20 1:22 ` Tiezhu Yang
@ 2024-02-20 8:56 ` Johan Almbladh
4 siblings, 0 replies; 6+ messages in thread
From: Johan Almbladh @ 2024-02-20 8:56 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Russell King, Puranjay Mohan, Zi Shen Lim, Catalin Marinas,
Will Deacon, Tiezhu Yang, Hengqi Chen, Huacai Chen, WANG Xuerui,
Paul Burton, Thomas Bogendoerfer, James E.J. Bottomley,
Helge Deller, Ilya Leoshkevich, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
David S. Miller, Andreas Larsson, Wang YanQing, David Ahern,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, bpf, linux-arm-kernel, linux-kernel, loongarch,
linux-mips, linux-parisc, linux-s390, sparclinux, netdev,
Kees Cook, linux-hardening @ vger . kernel . org
On Sun, Feb 18, 2024 at 11:55 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> set_memory_rox() can fail, leaving memory unprotected.
>
> Check return and bail out when bpf_jit_binary_lock_ro() returns
> and error.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Previous patch introduces a dependency on this patch because it modifies bpf_prog_lock_ro(), but they are independant.
> It is possible to apply this patch as standalone by handling trivial conflict with unmodified bpf_prog_lock_ro().
> ---
> arch/arm/net/bpf_jit_32.c | 25 ++++++++++++-------------
> arch/arm64/net/bpf_jit_comp.c | 21 +++++++++++++++------
> arch/loongarch/net/bpf_jit.c | 21 +++++++++++++++------
> arch/mips/net/bpf_jit_comp.c | 3 ++-
> arch/parisc/net/bpf_jit_core.c | 8 +++++++-
> arch/s390/net/bpf_jit_comp.c | 6 +++++-
> arch/sparc/net/bpf_jit_comp_64.c | 6 +++++-
> arch/x86/net/bpf_jit_comp32.c | 3 +--
> include/linux/filter.h | 4 ++--
> 9 files changed, 64 insertions(+), 33 deletions(-)
For the MIPS part:
Reviewed-by: Johan Almbladh <johan.almbladh@anyfinetworks.com>
Thanks,
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread