bpf: fix matching of data/data_end in verifier

The ctx structure passed into bpf programs is different depending on bpf program type. The verifier incorrectly marked ctx->data and ctx->data_end access based on ctx offset only. That caused loads in tracing programs int bpf_prog(struct pt_regs *ctx) { .. ctx->ax .. } to be incorrectly marked as PTR_TO_PACKET which later caused verifier to reject the program that was actually valid in tracing context. Fix this by doing program type specific matching of ctx offsets.

Alexei Starovoitov committed 10 years ago

Commit 19de99f1 parent e5826154 files · +52 −39Browse files

Unified · Split

4 files

include/linux/bpf.h +27 −1
kernel/bpf/verifier.c +7 −34
kernel/trace/bpf_trace.c +4 −2
net/core/filter.c +14 −2

include/linux/bpf.h+27−1

@@ -111,6 +111,31 @@ enum bpf_access_type { · 110 unmodified lines
111	BPF_WRITE = 2	111	BPF_WRITE = 2
112	};	112	};
113		113
		114	/* types of values stored in eBPF registers */
		115	enum bpf_reg_type {
		116	NOT_INIT = 0, /* nothing was written into register */
		117	UNKNOWN_VALUE, /* reg doesn't contain a valid pointer */
		118	PTR_TO_CTX, /* reg points to bpf_context */
		119	CONST_PTR_TO_MAP, /* reg points to struct bpf_map */
		120	PTR_TO_MAP_VALUE, /* reg points to map element value */
		121	PTR_TO_MAP_VALUE_OR_NULL,/* points to map elem value or NULL */
		122	FRAME_PTR, /* reg == frame_pointer */
		123	PTR_TO_STACK, /* reg == frame_pointer + imm */
		124	CONST_IMM, /* constant integer value */
		125
		126	/* PTR_TO_PACKET represents:
		127	* skb->data
		128	* skb->data + imm
		129	* skb->data + (u16) var
		130	* skb->data + (u16) var + imm
		131	* if (range > 0) then [ptr, ptr + range - off) is safe to access
		132	* if (id > 0) means that some 'var' was added
		133	* if (off > 0) menas that 'imm' was added
		134	*/
		135	PTR_TO_PACKET,
		136	PTR_TO_PACKET_END, /* skb->data + headlen */
		137	};
		138
114	struct bpf_prog;	139	struct bpf_prog;
115		140
116	struct bpf_verifier_ops {	141	struct bpf_verifier_ops {
@@ -120,7 +145,8 @@ struct bpf_verifier_ops { · 3 unmodified lines
120	/* return true if 'size' wide access at offset 'off' within bpf_context	145	/* return true if 'size' wide access at offset 'off' within bpf_context
121	* with 'type' (read or write) is allowed	146	* with 'type' (read or write) is allowed
122	*/	147	*/
123	bool (*is_valid_access)(int off, int size, enum bpf_access_type type);	148	bool (*is_valid_access)(int off, int size, enum bpf_access_type type,
		149	enum bpf_reg_type *reg_type);
124		150
125	u32 (*convert_ctx_access)(enum bpf_access_type type, int dst_reg,	151	u32 (*convert_ctx_access)(enum bpf_access_type type, int dst_reg,
126	int src_reg, int ctx_off,	152	int src_reg, int ctx_off,

kernel/bpf/verifier.c+7−34

  * are set to NOT_INIT to indicate that they are no longer readable.
  */
-/* types of values stored in eBPF registers */
-enum bpf_reg_type {
-	NOT_INIT = 0,		 /* nothing was written into register */
-	UNKNOWN_VALUE,		 /* reg doesn't contain a valid pointer */
-	PTR_TO_CTX,		 /* reg points to bpf_context */
-	CONST_PTR_TO_MAP,	 /* reg points to struct bpf_map */
-	PTR_TO_MAP_VALUE,	 /* reg points to map element value */
-	PTR_TO_MAP_VALUE_OR_NULL,/* points to map elem value or NULL */
-	FRAME_PTR,		 /* reg == frame_pointer */
-	PTR_TO_STACK,		 /* reg == frame_pointer + imm */
-	CONST_IMM,		 /* constant integer value */
-	/* PTR_TO_PACKET represents:
-	 * skb->data
-	 * skb->data + imm
-	 * skb->data + (u16) var
-	 * skb->data + (u16) var + imm
-	 * if (range > 0) then [ptr, ptr + range - off) is safe to access
-	 * if (id > 0) means that some 'var' was added
-	 * if (off > 0) menas that 'imm' was added
-	 */
-	PTR_TO_PACKET,
-	PTR_TO_PACKET_END,	 /* skb->data + headlen */
-};
 struct reg_state {
 	enum bpf_reg_type type;
 	union {
 /* check access to 'struct bpf_context' fields */
 static int check_ctx_access(struct verifier_env *env, int off, int size,
-			    enum bpf_access_type t)
+			    enum bpf_access_type t, enum bpf_reg_type *reg_type)
 {
 	if (env->prog->aux->ops->is_valid_access &&
-	    env->prog->aux->ops->is_valid_access(off, size, t)) {
+	    env->prog->aux->ops->is_valid_access(off, size, t, reg_type)) {
 		/* remember the offset of last byte accessed in ctx */
 		if (env->prog->aux->max_ctx_offset < off + size)
 			env->prog->aux->max_ctx_offset = off + size;
 			mark_reg_unknown_value(state->regs, value_regno);
 	} else if (reg->type == PTR_TO_CTX) {
+		enum bpf_reg_type reg_type = UNKNOWN_VALUE;
 		if (t == BPF_WRITE && value_regno >= 0 &&
 		    is_pointer_value(env, value_regno)) {
 			verbose("R%d leaks addr into ctx\n", value_regno);
 			return -EACCES;
 		}
-		err = check_ctx_access(env, off, size, t);
+		err = check_ctx_access(env, off, size, t, &reg_type);
 		if (!err && t == BPF_READ && value_regno >= 0) {
 			mark_reg_unknown_value(state->regs, value_regno);
-			if (off == offsetof(struct __sk_buff, data) &&
+			if (env->allow_ptr_leaks)
-			    env->allow_ptr_leaks)
 				/* note that reg.[id|off|range] == 0 */
-				state->regs[value_regno].type = PTR_TO_PACKET;
+				state->regs[value_regno].type = reg_type;
-			else if (off == offsetof(struct __sk_buff, data_end) &&
-				 env->allow_ptr_leaks)
-				state->regs[value_regno].type = PTR_TO_PACKET_END;
 		}
 	} else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {

kernel/trace/bpf_trace.c+4−2

 }
 /* bpf+kprobe programs can access fields of 'struct pt_regs' */
-static bool kprobe_prog_is_valid_access(int off, int size, enum bpf_access_type type)
+static bool kprobe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
+					enum bpf_reg_type *reg_type)
 {
 	/* check bounds */
 	if (off < 0 || off >= sizeof(struct pt_regs))
 	}
 }
-static bool tp_prog_is_valid_access(int off, int size, enum bpf_access_type type)
+static bool tp_prog_is_valid_access(int off, int size, enum bpf_access_type type,
+				    enum bpf_reg_type *reg_type)
 {
 	if (off < sizeof(void *) || off >= PERF_MAX_TRACE_SIZE)
 		return false;

net/core/filter.c+14−2

 }
 static bool sk_filter_is_valid_access(int off, int size,
-				      enum bpf_access_type type)
+				      enum bpf_access_type type,
+				      enum bpf_reg_type *reg_type)
 {
 	switch (off) {
 	case offsetof(struct __sk_buff, tc_classid):
 }
 static bool tc_cls_act_is_valid_access(int off, int size,
-				       enum bpf_access_type type)
+				       enum bpf_access_type type,
+				       enum bpf_reg_type *reg_type)
 {
 	if (type == BPF_WRITE) {
 		switch (off) {
 			return false;
 		}
 	}
+	switch (off) {
+	case offsetof(struct __sk_buff, data):
+		*reg_type = PTR_TO_PACKET;
+		break;
+	case offsetof(struct __sk_buff, data_end):
+		*reg_type = PTR_TO_PACKET_END;
+		break;
+	}
 	return __is_valid_access(off, size, type);
 }

bpf: fix matching of data/data_end in verifier

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code