Commit e0929922 authored by Paul Chaignon's avatar Paul Chaignon Committed by Martin KaFai Lau
Browse files

bpf: Reject narrower access to pointer ctx fields 



The following BPF program, simplified from a syzkaller repro, causes a
kernel warning:

    r0 = *(u8 *)(r1 + 169);
    exit;

With pointer field sk being at offset 168 in __sk_buff. This access is
detected as a narrower read in bpf_skb_is_valid_access because it
doesn't match offsetof(struct __sk_buff, sk). It is therefore allowed
and later proceeds to bpf_convert_ctx_access. Note that for the
"is_narrower_load" case in the convert_ctx_accesses(), the insn->off
is aligned, so the cnt may not be 0 because it matches the
offsetof(struct __sk_buff, sk) in the bpf_convert_ctx_access. However,
the target_size stays 0 and the verifier errors with a kernel warning:

    verifier bug: error during ctx access conversion(1)

This patch fixes that to return a proper "invalid bpf_context access
off=X size=Y" error on the load instruction.

The same issue affects multiple other fields in context structures that
allow narrow access. Some other non-affected fields (for sk_msg,
sk_lookup, and sockopt) were also changed to use bpf_ctx_range_ptr for
consistency.

Note this syzkaller crash was reported in the "Closes" link below, which
used to be about a different bug, fixed in
commit fce7bd8e ("bpf/verifier: Handle BPF_LOAD_ACQ instructions
in insn_def_regno()"). Because syzbot somehow confused the two bugs,
the new crash and repro didn't get reported to the mailing list.

Fixes: f96da094 ("bpf: simplify narrower ctx access")
Fixes: 0df1a55a ("bpf: Warn on internal verifier errors")
Reported-by: default avatar <syzbot+0ef84a7bdf5301d4cbec@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=0ef84a7bdf5301d4cbec


Signed-off-by: default avatarPaul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
Acked-by: default avatarEduard Zingerman <eddyz87@gmail.com>
Link: https://patch.msgid.link/3b8dcee67ff4296903351a974ddd9c4dca768b64.1753194596.git.paul.chaignon@gmail.com
parent 17ce3e59

kernel/bpf/cgroup.c

+4 −4
Original line number Diff line number Diff line
@@ -2440,22 +2440,22 @@ static bool cg_sockopt_is_valid_access(int off, int size, 
	}



	switch (off) {

	case offsetof(struct bpf_sockopt, sk):

	case bpf_ctx_range_ptr(struct bpf_sockopt, sk):

		if (size != sizeof(__u64))

			return false;

		info->reg_type = PTR_TO_SOCKET;

		break;

	case offsetof(struct bpf_sockopt, optval):

	case bpf_ctx_range_ptr(struct bpf_sockopt, optval):

		if (size != sizeof(__u64))

			return false;

		info->reg_type = PTR_TO_PACKET;

		break;

	case offsetof(struct bpf_sockopt, optval_end):

	case bpf_ctx_range_ptr(struct bpf_sockopt, optval_end):

		if (size != sizeof(__u64))

			return false;

		info->reg_type = PTR_TO_PACKET_END;

		break;

	case offsetof(struct bpf_sockopt, retval):

	case bpf_ctx_range(struct bpf_sockopt, retval):

		if (size != size_default)

			return false;

		return prog->expected_attach_type == BPF_CGROUP_GETSOCKOPT;

net/core/filter.c

+10 −10
Original line number Diff line number Diff line
@@ -8699,7 +8699,7 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type 
		if (size != sizeof(__u64))

			return false;

		break;

	case offsetof(struct __sk_buff, sk):

	case bpf_ctx_range_ptr(struct __sk_buff, sk):

		if (type == BPF_WRITE || size != sizeof(__u64))

			return false;

		info->reg_type = PTR_TO_SOCK_COMMON_OR_NULL;
@@ -9277,7 +9277,7 @@ static bool sock_addr_is_valid_access(int off, int size, 
				return false;

		}

		break;

	case offsetof(struct bpf_sock_addr, sk):

	case bpf_ctx_range_ptr(struct bpf_sock_addr, sk):

		if (type != BPF_READ)

			return false;

		if (size != sizeof(__u64))
@@ -9327,17 +9327,17 @@ static bool sock_ops_is_valid_access(int off, int size, 
			if (size != sizeof(__u64))

				return false;

			break;

		case offsetof(struct bpf_sock_ops, sk):

		case bpf_ctx_range_ptr(struct bpf_sock_ops, sk):

			if (size != sizeof(__u64))

				return false;

			info->reg_type = PTR_TO_SOCKET_OR_NULL;

			break;

		case offsetof(struct bpf_sock_ops, skb_data):

		case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data):

			if (size != sizeof(__u64))

				return false;

			info->reg_type = PTR_TO_PACKET;

			break;

		case offsetof(struct bpf_sock_ops, skb_data_end):

		case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data_end):

			if (size != sizeof(__u64))

				return false;

			info->reg_type = PTR_TO_PACKET_END;
@@ -9346,7 +9346,7 @@ static bool sock_ops_is_valid_access(int off, int size, 
			bpf_ctx_record_field_size(info, size_default);

			return bpf_ctx_narrow_access_ok(off, size,

							size_default);

		case offsetof(struct bpf_sock_ops, skb_hwtstamp):

		case bpf_ctx_range(struct bpf_sock_ops, skb_hwtstamp):

			if (size != sizeof(__u64))

				return false;

			break;
@@ -9416,17 +9416,17 @@ static bool sk_msg_is_valid_access(int off, int size, 
		return false;



	switch (off) {

	case offsetof(struct sk_msg_md, data):

	case bpf_ctx_range_ptr(struct sk_msg_md, data):

		info->reg_type = PTR_TO_PACKET;

		if (size != sizeof(__u64))

			return false;

		break;

	case offsetof(struct sk_msg_md, data_end):

	case bpf_ctx_range_ptr(struct sk_msg_md, data_end):

		info->reg_type = PTR_TO_PACKET_END;

		if (size != sizeof(__u64))

			return false;

		break;

	case offsetof(struct sk_msg_md, sk):

	case bpf_ctx_range_ptr(struct sk_msg_md, sk):

		if (size != sizeof(__u64))

			return false;

		info->reg_type = PTR_TO_SOCKET;
@@ -11632,7 +11632,7 @@ static bool sk_lookup_is_valid_access(int off, int size, 
		return false;



	switch (off) {

	case offsetof(struct bpf_sk_lookup, sk):

	case bpf_ctx_range_ptr(struct bpf_sk_lookup, sk):

		info->reg_type = PTR_TO_SOCKET_OR_NULL;

		return size == sizeof(__u64);