Skip to content

Commit

Permalink
bpf: sk_msg, improve offset chk in _is_valid_access
Browse files Browse the repository at this point in the history
The check for max offset in sk_msg_is_valid_access uses sizeof()
which is incorrect because it would allow accessing possibly
past the end of the struct in the padded case. Further, it doesn't
preclude accessing any padding that may be added in the middle of
a struct. All told this makes it fragile to rely on.

To fix this explicitly check offsets with fields using the
bpf_ctx_range() and bpf_ctx_range_till() macros.

For reference the current structure layout looks as follows (reported
by pahole)

struct sk_msg_md {
	union {
		void *             data;                 /*           8 */
	};                                               /*     0     8 */
	union {
		void *             data_end;             /*           8 */
	};                                               /*     8     8 */
	__u32                      family;               /*    16     4 */
	__u32                      remote_ip4;           /*    20     4 */
	__u32                      local_ip4;            /*    24     4 */
	__u32                      remote_ip6[4];        /*    28    16 */
	__u32                      local_ip6[4];         /*    44    16 */
	__u32                      remote_port;          /*    60     4 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	__u32                      local_port;           /*    64     4 */
	__u32                      size;                 /*    68     4 */

	/* size: 72, cachelines: 2, members: 10 */
	/* last cacheline: 8 bytes */
};

So there should be no padding at the moment but fixing this now
prevents future errors.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
  • Loading branch information
jrfastab authored and borkmann committed Dec 20, 2018
1 parent 9ee79a6 commit bc1b4f0
Showing 1 changed file with 14 additions and 7 deletions.
21 changes: 14 additions & 7 deletions net/core/filter.c
Original file line number Diff line number Diff line change
Expand Up @@ -6313,6 +6313,9 @@ static bool sk_msg_is_valid_access(int off, int size,
if (type == BPF_WRITE)
return false;

if (off % size != 0)
return false;

switch (off) {
case offsetof(struct sk_msg_md, data):
info->reg_type = PTR_TO_PACKET;
Expand All @@ -6324,16 +6327,20 @@ static bool sk_msg_is_valid_access(int off, int size,
if (size != sizeof(__u64))
return false;
break;
default:
case bpf_ctx_range(struct sk_msg_md, family):
case bpf_ctx_range(struct sk_msg_md, remote_ip4):
case bpf_ctx_range(struct sk_msg_md, local_ip4):
case bpf_ctx_range_till(struct sk_msg_md, remote_ip6[0], remote_ip6[3]):
case bpf_ctx_range_till(struct sk_msg_md, local_ip6[0], local_ip6[3]):
case bpf_ctx_range(struct sk_msg_md, remote_port):
case bpf_ctx_range(struct sk_msg_md, local_port):
case bpf_ctx_range(struct sk_msg_md, size):
if (size != sizeof(__u32))
return false;
}

if (off < 0 || off >= sizeof(struct sk_msg_md))
return false;
if (off % size != 0)
break;
default:
return false;

}
return true;
}

Expand Down

0 comments on commit bc1b4f0

Please sign in to comment.