linux-kernel - Re: [PATCH HID for-next v2 1/4] bpf: do not rely on ALLOW_ERROR_INJECTION for fmod

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y45py14LVP/bn2r5@macbook-pro-6.dhcp.thefacebook.com>
Date:   Mon, 5 Dec 2022 13:59:39 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Florent Revest <revest@...omium.org>,
        Jon Hunter <jonathanh@...dia.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH HID for-next v2 1/4] bpf: do not rely on
 ALLOW_ERROR_INJECTION for fmod_ret

On Mon, Dec 05, 2022 at 05:48:53PM +0100, Benjamin Tissoires wrote:
> The current way of expressing that a non-bpf kernel component is willing
> to accept that bpf programs can be attached to it and that they can change
> the return value is to abuse ALLOW_ERROR_INJECTION.
> This is debated in the link below, and the result is that it is not a
> reasonable thing to do.
> 
> Reuse the kfunc declaration structure to also tag the kernel functions
> we want to be fmodret. This way we can control from any subsystem which
> functions are being modified by bpf without touching the verifier.
> 
> 
> Link: https://lore.kernel.org/all/20221121104403.1545f9b5@gandalf.local.home/
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@...il.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> ---
>  include/linux/btf.h   |  3 +++
>  kernel/bpf/btf.c      | 41 +++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c | 17 +++++++++++++++--
>  net/bpf/test_run.c    | 14 +++++++++++---
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index f9aababc5d78..f71cfb20b9bf 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -412,8 +412,11 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>  u32 *btf_kfunc_id_set_contains(const struct btf *btf,
>  			       enum bpf_prog_type prog_type,
>  			       u32 kfunc_btf_id);
> +u32 *btf_kern_func_is_modify_return(const struct btf *btf,
> +				    u32 kfunc_btf_id);
>  int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
>  			      const struct btf_kfunc_id_set *s);
> +int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset);
>  s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id);
>  int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_cnt,
>  				struct module *owner);
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 35c07afac924..a22f3f45aca3 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -204,6 +204,7 @@ enum btf_kfunc_hook {
>  	BTF_KFUNC_HOOK_STRUCT_OPS,
>  	BTF_KFUNC_HOOK_TRACING,
>  	BTF_KFUNC_HOOK_SYSCALL,
> +	BTF_KFUNC_HOOK_FMODRET,
>  	BTF_KFUNC_HOOK_MAX,
>  };
>  
> @@ -7448,6 +7449,19 @@ u32 *btf_kfunc_id_set_contains(const struct btf *btf,
>  	return __btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id);
>  }
>  
> +/* Caution:
> + * Reference to the module (obtained using btf_try_get_module) corresponding to
> + * the struct btf *MUST* be held when calling this function from verifier
> + * context. This is usually true as we stash references in prog's kfunc_btf_tab;
> + * keeping the reference for the duration of the call provides the necessary
> + * protection for looking up a well-formed btf->kfunc_set_tab.
> + */

There is no need to copy paste that comment from btf_kfunc_id_set_contains().
One place is enough.

> +u32 *btf_kern_func_is_modify_return(const struct btf *btf,
> +				    u32 kfunc_btf_id)

How about btf_kfunc_is_modify_return ? 
For consistency.

> +{
> +	return __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_FMODRET, kfunc_btf_id);
> +}
> +
>  /* This function must be invoked only from initcalls/module init functions */
>  int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
>  			      const struct btf_kfunc_id_set *kset)
> @@ -7478,6 +7492,33 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
>  }
>  EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set);
>  
> +/* This function must be invoked only from initcalls/module init functions */
> +int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset)
> +{
> +	struct btf *btf;
> +	int ret;
> +
> +	btf = btf_get_module_btf(kset->owner);
> +	if (!btf) {
> +		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
> +			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
> +			return -ENOENT;
> +		}
> +		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) {
> +			pr_err("missing module BTF, cannot register kfuncs\n");
> +			return -ENOENT;
> +		}
> +		return 0;
> +	}
> +	if (IS_ERR(btf))
> +		return PTR_ERR(btf);
> +
> +	ret = btf_populate_kfunc_set(btf, BTF_KFUNC_HOOK_FMODRET, kset->set);
> +	btf_put(btf);
> +	return ret;
> +}

This is a bit too much copy-paste from register_btf_kfunc_id_set().
Please share the code. Like:

int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, const struct btf_kfunc_id_set *kset)
{
  ...
}

int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
                              const struct btf_kfunc_id_set *kset)
{
  hook = bpf_prog_type_to_kfunc_hook(prog_type);
  return __register_btf_kfunc_id_set(hook, kset);
}

int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset)
{
  return __register_btf_kfunc_id_set(BTF_KFUNC_HOOK_FMODRET, kset);
}