[PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory

[PATCH bpf-next v2 0/3] bpf: Prevent writing read-only memory

[Hao Luo]

Currently there are two ways to modify a kernel memory in bpf programs:
 1. declare a ksym of scalar type and directly modify its memory.
 2. Pass a RDONLY_BUF into a helper function which will override
 its arguments. For example, bpf_d_path, bpf_snprintf.

This patchset fixes these two problem. For the first, we introduce a
new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids
writing. Second, we introduce a new arg type ARG_PTR_TO_WRITABLE_MEM,
which is a proper subset of the ARG_PTR_TO_MEM and includes only those
reg types that are writable. For helper functions that may override its
argument, they should use ARG_PTR_TO_WRITABLE_MEM. For other helper
functions, they can continue using ARG_PTR_TO_MEM.

There is an alternative solution to the second problem, that is, an
ARG_PTR_TO_CONST_MEM, which represents the current ARG_PTR_TO_MEM, and
ARG_PTR_TO_MEM, which represents the ARG_PTR_TO_WRITABLE_MEM in this
patchset. But I find the naming here is too confusing. Most of the
helper functions should not override their arguments, therefore, using
ARG_PTR_TO_MEM sounds natural.

Hao Luo (3):
  bpf: Prevent write to ksym memory
  bpf: Introduce ARG_PTR_TO_WRITABLE_MEM
  bpf/selftests: Test PTR_TO_RDONLY_MEM

 include/linux/bpf.h                           | 15 +++++-
 include/uapi/linux/bpf.h                      |  4 +-
 kernel/bpf/cgroup.c                           |  2 +-
 kernel/bpf/helpers.c                          |  6 +--
 kernel/bpf/verifier.c                         | 54 ++++++++++++++++---
 kernel/trace/bpf_trace.c                      |  6 +--
 net/core/filter.c                             |  6 +--
 tools/include/uapi/linux/bpf.h                |  4 +-
 .../selftests/bpf/prog_tests/ksyms_btf.c      | 14 +++++
 .../bpf/progs/test_ksyms_btf_write_check.c    | 29 ++++++++++
 10 files changed, 116 insertions(+), 24 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c

-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH bpf-next 1/3] bpf: Prevent write to ksym memory

[Hao Luo]

A ksym could be used to refer a global variable in the kernel.
Previously if this variable is of non-struct type, bpf verifier resolves
its value type to be PTR_TO_MEM, which allows the program to write
to the memory. This patch introduces PTR_TO_RDONLY_MEM, which is
similar to PTR_TO_MEM, but forbids writing. This should prevent
program from writing kernel memory through ksyms.

Right now a PTR_TO_RDONLY_MEM can not be passed into any helper as
argument. But a PTR_TO_RDONLY_MEM can be read into a PTR_TO_MEM (e.g.
stack variable), which can be passed to helpers such as bpf_snprintf.

The following patch will add checks to differentiate the read-write
arguments and read-only arguments, and support for passing
PTR_TO_RDONLY_MEM to helper's read-only arguments.

Fixes: 63d9b80dcf2c ("bpf: Introducte bpf_this_cpu_ptr()")
Fixes: eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()")
Fixes: 4976b718c355 ("bpf: Introduce pseudo_btf_id")
Signed-off-by: Hao Luo <haoluo@google.com>
---
 Changes since v1:
  - Added Fixes tag.
  - Removed PTR_TO_RDONLY_MEM[_OR_NULL] from reg_type_may_be_refcounted.

 include/linux/bpf.h            |  6 ++++--
 include/uapi/linux/bpf.h       |  4 ++--
 kernel/bpf/helpers.c           |  4 ++--
 kernel/bpf/verifier.c          | 36 ++++++++++++++++++++++++++--------
 tools/include/uapi/linux/bpf.h |  4 ++--
 5 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 31421c74ba08..7b47e8f344cb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -355,8 +355,8 @@ enum bpf_return_type {
 	RET_PTR_TO_SOCK_COMMON_OR_NULL,	/* returns a pointer to a sock_common or NULL */
 	RET_PTR_TO_ALLOC_MEM_OR_NULL,	/* returns a pointer to dynamically allocated memory or NULL */
 	RET_PTR_TO_BTF_ID_OR_NULL,	/* returns a pointer to a btf_id or NULL */
-	RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
-	RET_PTR_TO_MEM_OR_BTF_ID,	/* returns a pointer to a valid memory or a btf_id */
+	RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a readonly memory or a btf_id or NULL */
+	RET_PTR_TO_RDONLY_MEM_OR_BTF_ID, /* returns a pointer to a readonly memory or a btf_id */
 	RET_PTR_TO_BTF_ID,		/* returns a pointer to a btf_id */
 };
 
@@ -459,6 +459,8 @@ enum bpf_reg_type {
 	PTR_TO_PERCPU_BTF_ID,	 /* reg points to a percpu kernel variable */
 	PTR_TO_FUNC,		 /* reg points to a bpf program function */
 	PTR_TO_MAP_KEY,		 /* reg points to a map element key */
+	PTR_TO_RDONLY_MEM,	 /* reg points to valid readonly memory region */
+	PTR_TO_RDONLY_MEM_OR_NULL, /* reg points to valid readonly memory region or null */
 	__BPF_REG_TYPE_MAX,
 };
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c10820037883..9fb931de668a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1141,8 +1141,8 @@ enum bpf_link_type {
  * insn[0].off:      0
  * insn[1].off:      0
  * ldimm64 rewrite:  address of the kernel variable
- * verifier type:    PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var
- *                   is struct/union.
+ * verifier type:    PTR_TO_BTF_ID or PTR_TO_RDONLY_MEM, depending on whether
+ *                   the var is struct/union.
  */
 #define BPF_PSEUDO_BTF_ID	3
 /* insn[0].src_reg:  BPF_PSEUDO_FUNC
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1ffd469c217f..14531757087f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -667,7 +667,7 @@ BPF_CALL_2(bpf_per_cpu_ptr, const void *, ptr, u32, cpu)
 const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
 	.func		= bpf_per_cpu_ptr,
 	.gpl_only	= false,
-	.ret_type	= RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL,
+	.ret_type	= RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL,
 	.arg1_type	= ARG_PTR_TO_PERCPU_BTF_ID,
 	.arg2_type	= ARG_ANYTHING,
 };
@@ -680,7 +680,7 @@ BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr)
 const struct bpf_func_proto bpf_this_cpu_ptr_proto = {
 	.func		= bpf_this_cpu_ptr,
 	.gpl_only	= false,
-	.ret_type	= RET_PTR_TO_MEM_OR_BTF_ID,
+	.ret_type	= RET_PTR_TO_RDONLY_MEM_OR_BTF_ID,
 	.arg1_type	= ARG_PTR_TO_PERCPU_BTF_ID,
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c6616e325803..ae3ff297240e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -453,6 +453,7 @@ static bool reg_type_may_be_null(enum bpf_reg_type type)
 	       type == PTR_TO_TCP_SOCK_OR_NULL ||
 	       type == PTR_TO_BTF_ID_OR_NULL ||
 	       type == PTR_TO_MEM_OR_NULL ||
+	       type == PTR_TO_RDONLY_MEM_OR_NULL ||
 	       type == PTR_TO_RDONLY_BUF_OR_NULL ||
 	       type == PTR_TO_RDWR_BUF_OR_NULL;
 }
@@ -571,6 +572,8 @@ static const char * const reg_type_str[] = {
 	[PTR_TO_PERCPU_BTF_ID]	= "percpu_ptr_",
 	[PTR_TO_MEM]		= "mem",
 	[PTR_TO_MEM_OR_NULL]	= "mem_or_null",
+	[PTR_TO_RDONLY_MEM]	= "rdonly_mem",
+	[PTR_TO_RDONLY_MEM_OR_NULL] = "rdonly_mem_or_null",
 	[PTR_TO_RDONLY_BUF]	= "rdonly_buf",
 	[PTR_TO_RDONLY_BUF_OR_NULL] = "rdonly_buf_or_null",
 	[PTR_TO_RDWR_BUF]	= "rdwr_buf",
@@ -1183,6 +1186,9 @@ static void mark_ptr_not_null_reg(struct bpf_reg_state *reg)
 	case PTR_TO_MEM_OR_NULL:
 		reg->type = PTR_TO_MEM;
 		break;
+	case PTR_TO_RDONLY_MEM_OR_NULL:
+		reg->type = PTR_TO_RDONLY_MEM;
+		break;
 	case PTR_TO_RDONLY_BUF_OR_NULL:
 		reg->type = PTR_TO_RDONLY_BUF;
 		break;
@@ -2741,6 +2747,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
 	case PTR_TO_PERCPU_BTF_ID:
 	case PTR_TO_MEM:
 	case PTR_TO_MEM_OR_NULL:
+	case PTR_TO_RDONLY_MEM:
+	case PTR_TO_RDONLY_MEM_OR_NULL:
 	case PTR_TO_FUNC:
 	case PTR_TO_MAP_KEY:
 		return true;
@@ -3367,6 +3375,7 @@ static int __check_mem_access(struct bpf_verifier_env *env, int regno,
 			off, size, regno, reg->id, off, mem_size);
 		break;
 	case PTR_TO_MEM:
+	case PTR_TO_RDONLY_MEM:
 	default:
 		verbose(env, "invalid access to memory, mem_size=%u off=%d size=%d\n",
 			mem_size, off, size);
@@ -4377,6 +4386,16 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 					      reg->mem_size, false);
 		if (!err && t == BPF_READ && value_regno >= 0)
 			mark_reg_unknown(env, regs, value_regno);
+	} else if (reg->type == PTR_TO_RDONLY_MEM) {
+		if (t == BPF_WRITE) {
+			verbose(env, "R%d cannot write into %s\n",
+				regno, reg_type_str[reg->type]);
+			return -EACCES;
+		}
+		err = check_mem_region_access(env, regno, off, size,
+					      reg->mem_size, false);
+		if (!err && value_regno >= 0)
+			mark_reg_unknown(env, regs, value_regno);
 	} else if (reg->type == PTR_TO_CTX) {
 		enum bpf_reg_type reg_type = SCALAR_VALUE;
 		struct btf *btf = NULL;
@@ -6532,8 +6551,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
 		regs[BPF_REG_0].mem_size = meta.mem_size;
-	} else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL ||
-		   fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) {
+	} else if (fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID_OR_NULL ||
+		   fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID) {
 		const struct btf_type *t;
 
 		mark_reg_known_zero(env, regs, BPF_REG_0);
@@ -6552,12 +6571,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 				return -EINVAL;
 			}
 			regs[BPF_REG_0].type =
-				fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ?
-				PTR_TO_MEM : PTR_TO_MEM_OR_NULL;
+				fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID ?
+				PTR_TO_RDONLY_MEM : PTR_TO_RDONLY_MEM_OR_NULL;
 			regs[BPF_REG_0].mem_size = tsize;
 		} else {
 			regs[BPF_REG_0].type =
-				fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ?
+				fn->ret_type == RET_PTR_TO_RDONLY_MEM_OR_BTF_ID ?
 				PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL;
 			regs[BPF_REG_0].btf = meta.ret_btf;
 			regs[BPF_REG_0].btf_id = meta.ret_btf_id;
@@ -9363,7 +9382,7 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
 
 		dst_reg->type = aux->btf_var.reg_type;
 		switch (dst_reg->type) {
-		case PTR_TO_MEM:
+		case PTR_TO_RDONLY_MEM:
 			dst_reg->mem_size = aux->btf_var.mem_size;
 			break;
 		case PTR_TO_BTF_ID:
@@ -11510,7 +11529,7 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 			err = -EINVAL;
 			goto err_put;
 		}
-		aux->btf_var.reg_type = PTR_TO_MEM;
+		aux->btf_var.reg_type = PTR_TO_RDONLY_MEM;
 		aux->btf_var.mem_size = tsize;
 	} else {
 		aux->btf_var.reg_type = PTR_TO_BTF_ID;
@@ -13332,7 +13351,8 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 				mark_reg_known_zero(env, regs, i);
 			else if (regs[i].type == SCALAR_VALUE)
 				mark_reg_unknown(env, regs, i);
-			else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
+			else if (regs[i].type == PTR_TO_MEM_OR_NULL ||
+				 regs[i].type == PTR_TO_RDONLY_MEM_OR_NULL) {
 				const u32 mem_size = regs[i].mem_size;
 
 				mark_reg_known_zero(env, regs, i);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c10820037883..9fb931de668a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1141,8 +1141,8 @@ enum bpf_link_type {
  * insn[0].off:      0
  * insn[1].off:      0
  * ldimm64 rewrite:  address of the kernel variable
- * verifier type:    PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var
- *                   is struct/union.
+ * verifier type:    PTR_TO_BTF_ID or PTR_TO_RDONLY_MEM, depending on whether
+ *                   the var is struct/union.
  */
 #define BPF_PSEUDO_BTF_ID	3
 /* insn[0].src_reg:  BPF_PSEUDO_FUNC
-- 
2.33.0.1079.g6e70778dc9-goog

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

[Hao Luo]

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.
+					    */
 	__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

[PATCH bpf-next v2 3/3] bpf/selftests: Test PTR_TO_RDONLY_MEM

[Hao Luo]

This test verifies that a ksym of non-struct can not be directly
updated.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 Changes since v1:
  - Replaced CHECK() with ASSERT_ERR_PTR()
  - Commented in the test for the reason of verification failure.

 .../selftests/bpf/prog_tests/ksyms_btf.c      | 14 +++++++++
 .../bpf/progs/test_ksyms_btf_write_check.c    | 29 +++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
index cf3acfa5a91d..69455fe90ac3 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
@@ -7,6 +7,7 @@
 #include "test_ksyms_btf.skel.h"
 #include "test_ksyms_btf_null_check.skel.h"
 #include "test_ksyms_weak.skel.h"
+#include "test_ksyms_btf_write_check.skel.h"
 
 static int duration;
 
@@ -109,6 +110,16 @@ static void test_weak_syms(void)
 	test_ksyms_weak__destroy(skel);
 }
 
+static void test_write_check(void)
+{
+	struct test_ksyms_btf_write_check *skel;
+
+	skel = test_ksyms_btf_write_check__open_and_load();
+	ASSERT_ERR_PTR(skel, "unexpected load of a prog writing to ksym memory\n");
+
+	test_ksyms_btf_write_check__destroy(skel);
+}
+
 void test_ksyms_btf(void)
 {
 	int percpu_datasec;
@@ -136,4 +147,7 @@ void test_ksyms_btf(void)
 
 	if (test__start_subtest("weak_ksyms"))
 		test_weak_syms();
+
+	if (test__start_subtest("write_check"))
+		test_write_check();
 }
diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c b/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c
new file mode 100644
index 000000000000..2180c41cd890
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Google */
+
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+
+extern const int bpf_prog_active __ksym; /* int type global var. */
+
+SEC("raw_tp/sys_enter")
+int handler(const void *ctx)
+{
+	int *active;
+	__u32 cpu;
+
+	cpu = bpf_get_smp_processor_id();
+	active = (int *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
+	if (active) {
+		/* Kernel memory obtained from bpf_{per,this}_cpu_ptr
+		 * is read-only, should _not_ pass verification.
+		 */
+		/* WRITE_ONCE */
+		*(volatile int *)active = -1;
+	}
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.33.0.1079.g6e70778dc9-goog

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

[Alexei Starovoitov]

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;

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

[Andrii Nakryiko]

On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> 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@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.
> +                                           */
>         __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
>

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

[Andrii Nakryiko]

On Mon, Oct 25, 2021 at 8:48 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> 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 */

This seems more confusing, it's technically possible to express a
memory pointer from which you can read X bytes, but can write Y bytes.

I actually liked the idea that helpers will be explicit about whether
they can write into a memory or only read from it.

Apart from a few more lines of code, are there any downsides to having
PTR_TO_MEM vs PTR_TO_RDONLY_MEM?

BTW, this made me think about read-only (from the BPF side) maps.
Seems like we have some special handling around that right now, but if
we had PTR_TO_RDONLY_MEM and PTR_TO_MEM, could we have used that as a
more uniform way to enforce read-only access to memory?

>
>                 /* 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;
>

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

[Hao Luo]

On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> 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.
>

My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I
find it's not intuitive for developers when adding helpers. The
majority of PTR_TO_MEM arguments are read-only. When adding a new
helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM)
to match the default case (that is, read-only argument). Requiring
explicitly adding RDONLY to most cases seems a little unintuitive to
me.

But other than that, I agree that narrowing ARG_PTR_TO_MEM down to
writable memory fosters more strict checks and safer behavior. But
when people add helpers, they are clearly aware which argument will be
modified and which will not. IMHO I do trust that the developers and
the reviewers can choose the right type at the review time. :)

> >
> > 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.
> > +                                           */
> >         __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
> >

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

[Alexei Starovoitov]

On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> >
> > 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 */
>
> This seems more confusing, it's technically possible to express a
> memory pointer from which you can read X bytes, but can write Y bytes.

I'm fine it being a new flag instead of wr_mem_size.

> I actually liked the idea that helpers will be explicit about whether
> they can write into a memory or only read from it.
>
> Apart from a few more lines of code, are there any downsides to having
> PTR_TO_MEM vs PTR_TO_RDONLY_MEM?

because it's a churn and non scalable long term.
It's not just PTR_TO_RDONLY_MEM.
It's also ARG_PTR_TO_RDONLY_MEM,
and RET_PTR_TO_RDONLY_MEM,
and PTR_TO_RDONLY_MEM_OR_NULL
and *_OR_BTF_ID,
and *_OR_BTF_ID_OR_NULL.
It felt that expressing readonly-ness as a flag in bpf_reg_state
will make it easier to get right in the code and extend in the future.
May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
If we use different name to express that we will have:
PTR_TO_USER_RDONLY_MEM and
PTR_TO_USER_MEM
plus all variants of ARG_* and RET_* and *_OR_NULL.
With a flag approach it will be just another flag in bpf_reg_state.

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

[Hao Luo]

On Tue, Oct 26, 2021 at 11:00 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >
> > > 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 */
> >
> > This seems more confusing, it's technically possible to express a
> > memory pointer from which you can read X bytes, but can write Y bytes.
>
> I'm fine it being a new flag instead of wr_mem_size.
>
> > I actually liked the idea that helpers will be explicit about whether
> > they can write into a memory or only read from it.
> >
> > Apart from a few more lines of code, are there any downsides to having
> > PTR_TO_MEM vs PTR_TO_RDONLY_MEM?
>
> because it's a churn and non scalable long term.
> It's not just PTR_TO_RDONLY_MEM.
> It's also ARG_PTR_TO_RDONLY_MEM,
> and RET_PTR_TO_RDONLY_MEM,
> and PTR_TO_RDONLY_MEM_OR_NULL
> and *_OR_BTF_ID,
> and *_OR_BTF_ID_OR_NULL.
> It felt that expressing readonly-ness as a flag in bpf_reg_state
> will make it easier to get right in the code and extend in the future.
> May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
> If we use different name to express that we will have:
> PTR_TO_USER_RDONLY_MEM and
> PTR_TO_USER_MEM
> plus all variants of ARG_* and RET_* and *_OR_NULL.
> With a flag approach it will be just another flag in bpf_reg_state.

Totally agree. Adding a variant incurs exponential cost. Introducing
another dimension in future may need to go over all the MEM,
RDONLY_MEM, MEM_OR_NULL, RDONLY_MEM_OR_NULL, multiplied by ARG_*,
RET_*, etc. It's a pain.

I have that in mind and start thinking more about how can we do a more
scalable flag approach.

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

[Andrii Nakryiko]

On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >
> > > 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 */
> >
> > This seems more confusing, it's technically possible to express a
> > memory pointer from which you can read X bytes, but can write Y bytes.
>
> I'm fine it being a new flag instead of wr_mem_size.
>
> > I actually liked the idea that helpers will be explicit about whether
> > they can write into a memory or only read from it.
> >
> > Apart from a few more lines of code, are there any downsides to having
> > PTR_TO_MEM vs PTR_TO_RDONLY_MEM?
>
> because it's a churn and non scalable long term.
> It's not just PTR_TO_RDONLY_MEM.
> It's also ARG_PTR_TO_RDONLY_MEM,
> and RET_PTR_TO_RDONLY_MEM,
> and PTR_TO_RDONLY_MEM_OR_NULL
> and *_OR_BTF_ID,
> and *_OR_BTF_ID_OR_NULL.
> It felt that expressing readonly-ness as a flag in bpf_reg_state
> will make it easier to get right in the code and extend in the future.

That's true, but while it's easy to add a flag to bpf_reg_state, it's
not easy to do the same for BPF helper input (ARG_PTR_xxx) and output
(RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR
with flags, it seems more consistent to keep the same pure enum
approach for reg_state.

> May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
> If we use different name to express that we will have:
> PTR_TO_USER_RDONLY_MEM and
> PTR_TO_USER_MEM
> plus all variants of ARG_* and RET_* and *_OR_NULL.
> With a flag approach it will be just another flag in bpf_reg_state.

All true, but then maybe we should rethink how we do all those enums.
And instead of having all the _OR_NULL variants, it should be
ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the
basic set of register/input/output type enums? Same for ARG_RDONLY
flag. Same could technically be done for USER vs KERNEL memory in the
future.

It's definitely a bunch of code changes, but if we are worried about
an explosion of enum values, it might be the right move?

On the other hand, if there are all those different variations and
each is handled slightly differently, we'll have to have different
logic for each of them. And whether it's an enum + flags, or a few
more enumerators, doesn't change anything fundamentally. I feel like
enums make code discovery a bit simpler in practice, but it's
subjective.

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

[Andrii Nakryiko]

On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote:
>
> On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> 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.
> >
>
> My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I
> find it's not intuitive for developers when adding helpers. The
> majority of PTR_TO_MEM arguments are read-only. When adding a new
> helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM)
> to match the default case (that is, read-only argument). Requiring
> explicitly adding RDONLY to most cases seems a little unintuitive to
> me.
>
> But other than that, I agree that narrowing ARG_PTR_TO_MEM down to
> writable memory fosters more strict checks and safer behavior. But
> when people add helpers, they are clearly aware which argument will be
> modified and which will not. IMHO I do trust that the developers and
> the reviewers can choose the right type at the review time. :)

I don't trust myself, and neither should you :) See 5b029a32cfe4
("bpf: Fix ringbuf helper function compatibility") as an example of
the things that shouldn't have happened, but slipped through my own
testing and code review anyway. And there were multiple cases where we
accidentally enabled stuff that we shouldn't or didn't check something
that should have been prevented.

All that is to say that if we can have safer behavior by default (not
as enforced by humans), then it's better. In this sense,
ARG_PTR_TO_MEM meaning writable access is safer, because even if we
accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst
thing is that users won't be able to use helper in some situation and
hopefully will report this and we'll fix it. The alternative is that a
helper declares the argument as read-only memory (accidentally,
because it's shorter enum and sort of default), but actually does the
write to that memory. Now we have a much bigger issue.

>
> > >
> > > 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.
> > >

[...]

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

[Alexei Starovoitov]

On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > 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 */
> > >
> > > This seems more confusing, it's technically possible to express a
> > > memory pointer from which you can read X bytes, but can write Y bytes.
> >
> > I'm fine it being a new flag instead of wr_mem_size.
> >
> > > I actually liked the idea that helpers will be explicit about whether
> > > they can write into a memory or only read from it.
> > >
> > > Apart from a few more lines of code, are there any downsides to having
> > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM?
> >
> > because it's a churn and non scalable long term.
> > It's not just PTR_TO_RDONLY_MEM.
> > It's also ARG_PTR_TO_RDONLY_MEM,
> > and RET_PTR_TO_RDONLY_MEM,
> > and PTR_TO_RDONLY_MEM_OR_NULL
> > and *_OR_BTF_ID,
> > and *_OR_BTF_ID_OR_NULL.
> > It felt that expressing readonly-ness as a flag in bpf_reg_state
> > will make it easier to get right in the code and extend in the future.
>
> That's true, but while it's easy to add a flag to bpf_reg_state, it's
> not easy to do the same for BPF helper input (ARG_PTR_xxx) and output
> (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR
> with flags, it seems more consistent to keep the same pure enum
> approach for reg_state.
>
> > May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
> > If we use different name to express that we will have:
> > PTR_TO_USER_RDONLY_MEM and
> > PTR_TO_USER_MEM
> > plus all variants of ARG_* and RET_* and *_OR_NULL.
> > With a flag approach it will be just another flag in bpf_reg_state.
>
> All true, but then maybe we should rethink how we do all those enums.
> And instead of having all the _OR_NULL variants, it should be
> ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the
> basic set of register/input/output type enums? Same for ARG_RDONLY
> flag. Same could technically be done for USER vs KERNEL memory in the
> future.

Exactly. OR_NULL is such a flag and we already struggled to
differentiate that flag with truly_not_equal_to_NULL and may_be_NULL.
That's why all bpf_skc* helpers have additional run-time !NULL check.

ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner.
And ARG_RDONLY would fit that model well.

> It's definitely a bunch of code changes, but if we are worried about
> an explosion of enum values, it might be the right move?
>
> On the other hand, if there are all those different variations and
> each is handled slightly differently, we'll have to have different
> logic for each of them. And whether it's an enum + flags, or a few
> more enumerators, doesn't change anything fundamentally. I feel like
> enums make code discovery a bit simpler in practice, but it's
> subjective.

I think it's a bit of a mess already.
ARG_PTR_TO_BTF_ID has may_be_NULL flag.
Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON.
but RET_PTR_TO_BTF_ID doesn't.
PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either.

imo cleaning up OR_NULL will be very nice.
RDONLY would be an addition on top.
We can probably fold UNINIT as a flag too.

All that will be a big change, but I think it's worth it.

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

[Hao Luo]

On Tue, Oct 26, 2021 at 11:53 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote:
> >
> > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> 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.
> > >
> >
> > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I
> > find it's not intuitive for developers when adding helpers. The
> > majority of PTR_TO_MEM arguments are read-only. When adding a new
> > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM)
> > to match the default case (that is, read-only argument). Requiring
> > explicitly adding RDONLY to most cases seems a little unintuitive to
> > me.
> >
> > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to
> > writable memory fosters more strict checks and safer behavior. But
> > when people add helpers, they are clearly aware which argument will be
> > modified and which will not. IMHO I do trust that the developers and
> > the reviewers can choose the right type at the review time. :)
>
> I don't trust myself, and neither should you :) See 5b029a32cfe4
> ("bpf: Fix ringbuf helper function compatibility") as an example of
> the things that shouldn't have happened, but slipped through my own
> testing and code review anyway. And there were multiple cases where we
> accidentally enabled stuff that we shouldn't or didn't check something
> that should have been prevented.
>
> All that is to say that if we can have safer behavior by default (not
> as enforced by humans), then it's better. In this sense,
> ARG_PTR_TO_MEM meaning writable access is safer, because even if we
> accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst
> thing is that users won't be able to use helper in some situation and
> hopefully will report this and we'll fix it. The alternative is that a
> helper declares the argument as read-only memory (accidentally,
> because it's shorter enum and sort of default), but actually does the
> write to that memory. Now we have a much bigger issue.
>

Acknowledged. Will make this change in this next iteration.

> >
> > > >
> > > > 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.
> > > >
>
> [...]

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

[Andrii Nakryiko]

On Tue, Oct 26, 2021 at 12:23 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > > >
> > > > > 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 */
> > > >
> > > > This seems more confusing, it's technically possible to express a
> > > > memory pointer from which you can read X bytes, but can write Y bytes.
> > >
> > > I'm fine it being a new flag instead of wr_mem_size.
> > >
> > > > I actually liked the idea that helpers will be explicit about whether
> > > > they can write into a memory or only read from it.
> > > >
> > > > Apart from a few more lines of code, are there any downsides to having
> > > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM?
> > >
> > > because it's a churn and non scalable long term.
> > > It's not just PTR_TO_RDONLY_MEM.
> > > It's also ARG_PTR_TO_RDONLY_MEM,
> > > and RET_PTR_TO_RDONLY_MEM,
> > > and PTR_TO_RDONLY_MEM_OR_NULL
> > > and *_OR_BTF_ID,
> > > and *_OR_BTF_ID_OR_NULL.
> > > It felt that expressing readonly-ness as a flag in bpf_reg_state
> > > will make it easier to get right in the code and extend in the future.
> >
> > That's true, but while it's easy to add a flag to bpf_reg_state, it's
> > not easy to do the same for BPF helper input (ARG_PTR_xxx) and output
> > (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR
> > with flags, it seems more consistent to keep the same pure enum
> > approach for reg_state.
> >
> > > May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
> > > If we use different name to express that we will have:
> > > PTR_TO_USER_RDONLY_MEM and
> > > PTR_TO_USER_MEM
> > > plus all variants of ARG_* and RET_* and *_OR_NULL.
> > > With a flag approach it will be just another flag in bpf_reg_state.
> >
> > All true, but then maybe we should rethink how we do all those enums.
> > And instead of having all the _OR_NULL variants, it should be
> > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the
> > basic set of register/input/output type enums? Same for ARG_RDONLY
> > flag. Same could technically be done for USER vs KERNEL memory in the
> > future.
>
> Exactly. OR_NULL is such a flag and we already struggled to
> differentiate that flag with truly_not_equal_to_NULL and may_be_NULL.
> That's why all bpf_skc* helpers have additional run-time !NULL check.
>
> ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner.
> And ARG_RDONLY would fit that model well.
>
> > It's definitely a bunch of code changes, but if we are worried about
> > an explosion of enum values, it might be the right move?
> >
> > On the other hand, if there are all those different variations and
> > each is handled slightly differently, we'll have to have different
> > logic for each of them. And whether it's an enum + flags, or a few
> > more enumerators, doesn't change anything fundamentally. I feel like
> > enums make code discovery a bit simpler in practice, but it's
> > subjective.
>
> I think it's a bit of a mess already.
> ARG_PTR_TO_BTF_ID has may_be_NULL flag.
> Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON.
> but RET_PTR_TO_BTF_ID doesn't.
> PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either.
>
> imo cleaning up OR_NULL will be very nice.
> RDONLY would be an addition on top.
> We can probably fold UNINIT as a flag too.
>
> All that will be a big change, but I think it's worth it.

Yep, agree.