On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Some helper functions may modify its arguments, for example,
> bpf_d_path, bpf_get_stack etc. Previously, their argument types
> were marked as ARG_PTR_TO_MEM, which is compatible with read-only
> mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate
> to modify a read-only memory by passing it into one of such helper
> functions.
>
> This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to
> annotate the arguments that may be modified by the helpers. For
> arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type,
> while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not
> acceptable.
>
> In short, when a helper may modify its input parameter, use
> ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM.
This is inconsistent with the other uses where we have something
that's writable by default and mark it as RDONLY if it's read-only.
Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and
add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the
memory? It will also be safer default: if helper defines
ARG_PTR_TO_MEM but never writes to it, worst thing that can happen
would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it
without fixing helper definition. The other way is more dangerous. If
ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction
and actually writes into the memory, then we are in much bigger
trouble.
>
> So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM
> is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is
> only used in bpf_iter prog as the type of key, which hasn't been
> used in the affected helper functions. PTR_TO_RDONLY_MEM currently
> has no consumers.
>
> Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> ---
> Changes since v1:
> - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate
> read-only and read-write mem arg types.
>
> include/linux/bpf.h | 9 +++++++++
> kernel/bpf/cgroup.c | 2 +-
> kernel/bpf/helpers.c | 2 +-
> kernel/bpf/verifier.c | 18 ++++++++++++++++++
> kernel/trace/bpf_trace.c | 6 +++---
> net/core/filter.c | 6 +++---
> 6 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 7b47e8f344cb..586ce67d63a9 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -341,6 +341,15 @@ enum bpf_arg_type {
> ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */
> ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */
> ARG_PTR_TO_TIMER, /* pointer to bpf_timer */
> + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to
> + * ARG_PTR_TO_MEM, this arg_type is not
> + * compatible with RDONLY memory. If the
> + * argument may be updated by the helper,
> + * use this type.
> + */
> + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to
> + * ARG_PTR_TO_WRITABLE_MEM.
> + */
> __BPF_ARG_TYPE_MAX,
> };
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 03145d45e3d5..683269b7cd92 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = {
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_MEM,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg3_type = ARG_CONST_SIZE,
> .arg4_type = ARG_ANYTHING,
> };
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 14531757087f..db98385ee7af 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = {
> .func = bpf_snprintf,
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> - .arg1_type = ARG_PTR_TO_MEM_OR_NULL,
> + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL,
> .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> .arg3_type = ARG_PTR_TO_CONST_STR,
> .arg4_type = ARG_PTR_TO_MEM_OR_NULL,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ae3ff297240e..d8817c3d990e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type)
> type == ARG_PTR_TO_CTX_OR_NULL ||
> type == ARG_PTR_TO_SOCKET_OR_NULL ||
> type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
> + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL ||
> type == ARG_PTR_TO_STACK_OR_NULL;
> }
>
> @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type)
> {
> return type == ARG_PTR_TO_MEM ||
> type == ARG_PTR_TO_MEM_OR_NULL ||
> + type == ARG_PTR_TO_WRITABLE_MEM ||
> + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL ||
> type == ARG_PTR_TO_UNINIT_MEM;
> }
>
> @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = {
> PTR_TO_MEM,
> PTR_TO_RDONLY_BUF,
> PTR_TO_RDWR_BUF,
> + PTR_TO_RDONLY_MEM,
> + },
> +};
> +
> +static const struct bpf_reg_types writable_mem_types = {
> + .types = {
> + PTR_TO_STACK,
> + PTR_TO_PACKET,
> + PTR_TO_PACKET_META,
> + PTR_TO_MAP_KEY,
> + PTR_TO_MAP_VALUE,
> + PTR_TO_MEM,
> + PTR_TO_RDWR_BUF,
> },
> };
>
> @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> [ARG_PTR_TO_UNINIT_MEM] = &mem_types,
> [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types,
> [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types,
> + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types,
> + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types,
> [ARG_PTR_TO_INT] = &int_ptr_types,
> [ARG_PTR_TO_LONG] = &int_ptr_types,
> [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types,
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cbcd0d6fca7c..5815845222de 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_BTF_ID,
> .arg1_btf_id = &bpf_d_path_btf_ids[0],
> - .arg2_type = ARG_PTR_TO_MEM,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> .allowed = bpf_d_path_allowed,
> };
> @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
> .func = bpf_snprintf_btf,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> - .arg1_type = ARG_PTR_TO_MEM,
> + .arg1_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg2_type = ARG_CONST_SIZE,
> .arg3_type = ARG_PTR_TO_MEM,
> .arg4_type = ARG_CONST_SIZE,
> @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_MEM_OR_NULL,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL,
> .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> .arg4_type = ARG_ANYTHING,
> };
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8e8d3b49c297..7dadabe12ceb 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_MEM,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg3_type = ARG_CONST_SIZE,
> .arg4_type = ARG_ANYTHING,
> };
> @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_MEM,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg3_type = ARG_CONST_SIZE,
> .arg4_type = ARG_ANYTHING,
> };
> @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = {
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_MEM,
> + .arg2_type = ARG_PTR_TO_WRITABLE_MEM,
> .arg3_type = ARG_CONST_SIZE,
> .arg4_type = ARG_ANYTHING,
> };
> --
> 2.33.0.1079.g6e70778dc9-goog
>
