Re: [PATCH bpf-next 2/2] bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()

[Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>]

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@xxxxxxxxxx>
> ---

...

> 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@xxxxxxxxxxx>  # LoongArch

Thanks,
Tiezhu