Re: [PATCH bpf-next v2 2/3] bpf: Introduce ARG_PTR_TO_WRITABLE_MEM

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Oct 25, 2021 at 04:12:55PM -0700, Hao Luo 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.
> 
> 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@google.com>
> ---
>  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.
> +					    */

Instead of adding new types,
can we do something like this instead:

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c8a78e830fca..5dbd2541aa86 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -68,7 +68,8 @@ struct bpf_reg_state {
                        u32 btf_id;
                };

-               u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
+               u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
+               u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */

                /* Max size from any of the above. */
                struct {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c6616e325803..ad46169d422b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4374,7 +4374,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
                        return -EACCES;
                }
                err = check_mem_region_access(env, regno, off, size,
-                                             reg->mem_size, false);
+                                             t == BPF_WRITE ? reg->wr_mem_size : reg->rd_mem_size, false);
                if (!err && t == BPF_READ && value_regno >= 0)
                        mark_reg_unknown(env, regs, value_regno);
        } else if (reg->type == PTR_TO_CTX) {
@@ -11511,7 +11511,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
                        goto err_put;
                }
                aux->btf_var.reg_type = PTR_TO_MEM;
-               aux->btf_var.mem_size = tsize;
+               aux->btf_var.rd_mem_size = tsize;
+               aux->btf_var.wr_mem_size = 0;
        } else {
                aux->btf_var.reg_type = PTR_TO_BTF_ID;
                aux->btf_var.btf = btf;