From: John Fastabend Subject: [PATCH bpf-next 2/8] bpf: sk_msg, improve offset chk in _is_valid_access Date: Thu, 20 Dec 2018 11:35:30 -0800 Message-ID: <1545334536-766-3-git-send-email-john.fastabend@gmail.com> References: <1545334536-766-1-git-send-email-john.fastabend@gmail.com> Cc: netdev@vger.kernel.org, john.fastabend@gmail.com To: ast@kernel.org, daniel@iogearbox.net Return-path: Received: from mail-it1-f196.google.com ([209.85.166.196]:50700 "EHLO mail-it1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725785AbeLTTf7 (ORCPT ); Thu, 20 Dec 2018 14:35:59 -0500 Received: by mail-it1-f196.google.com with SMTP id z7so3724914iti.0 for ; Thu, 20 Dec 2018 11:35:58 -0800 (PST) In-Reply-To: <1545334536-766-1-git-send-email-john.fastabend@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 Signed-off-by: John Fastabend --- net/core/filter.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 3a3b217..6bd9f08 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -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; @@ -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; } -- 2.7.4