From patchwork Fri Sep 13 19:17:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Borkmann X-Patchwork-Id: 13803972 X-Patchwork-Delegate: bpf@iogearbox.net Received: from www62.your-server.de (www62.your-server.de [213.133.104.62]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 10C3B146581 for ; Fri, 13 Sep 2024 19:18:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.133.104.62 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726255088; cv=none; b=cUmIYL85mUvD21UAhs4x9nJS9gk4+2IZYNoA3MVq5z45mlzLYKFp7bHdtbKBywMmCLMNBXHtXpjk8rnCkduCwxwu9M5qKcsXT20Ip+MJySTAy6/2J6BQbBLfRs8YDVUglJJVOXuF26pvOhXbcG0KxKKwnE+2iFOzDVztM3F3Zdc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726255088; c=relaxed/simple; bh=hL6vpFzSkmXu8Y1Cv7cfFZDCsswhWbILXsbzCtMwFCw=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=DfUPc3IiwDyV2DE1jrb0D+F9sEkXfLtKNqPCD6Z5XytTw0JouqijtGPXn8mZZe5w2kV4Dk8PJX6M+Zbr0xjcFC8efSSflG36xC2nO6smOWi18CbX6/ZvGeugqD5MOdrBXTSPcYbOygqiC34qfG8xo3maaGqHpAAF7qehvxYahcg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=iogearbox.net; spf=pass smtp.mailfrom=iogearbox.net; dkim=pass (2048-bit key) header.d=iogearbox.net header.i=@iogearbox.net header.b=pxQFf8iu; arc=none smtp.client-ip=213.133.104.62 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=iogearbox.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=iogearbox.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=iogearbox.net header.i=@iogearbox.net header.b="pxQFf8iu" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=iogearbox.net; s=default2302; h=Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: Content-Type:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=C7oss+qF5Jv4m1jsl4ZAubUeIwBXl4M6JFAuor2i3uc=; b=pxQFf8iu50hM03Fbh+LDNRTiwq 2dAqYl2eDOf9zRK4frbjPEdjSxZTppyswAVR/8L8A3FYnPoH3KZoeQ3V/4C2NaY/lODl+/Rh3AY7l MVhparM8I0yGtvKtC083ofB1IidX9ySi1+wdyqi4P556wP9pcYTaGtS0Kax5AWm5ZII4l82UNporA QorsjFQ+iE7drDp7AayZJPTE8gcpRcOYB5vkY8Yj++ISquOj94w1ggFpdNmmhECL8NgMaiB1/hCnC 17soNZlqO92qqw4JGi5YwyrMTRQfzNU9JMSafebxSJAcKFkfNr2H8w25gCkuGmqbLmqy4fhHziNIr 4jIR4FUw==; Received: from 43.249.197.178.dynamic.cust.swisscom.net ([178.197.249.43] helo=localhost) by www62.your-server.de with esmtpsa (TLS1.3) tls TLS_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1spBnq-000KtE-Re; Fri, 13 Sep 2024 21:17:58 +0200 From: Daniel Borkmann To: bpf@vger.kernel.org Cc: shung-hsi.yu@suse.com, andrii@kernel.org, ast@kernel.org, kongln9170@gmail.com, Daniel Borkmann Subject: [PATCH bpf-next v5 5/9] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error Date: Fri, 13 Sep 2024 21:17:50 +0200 Message-Id: <20240913191754.13290-5-daniel@iogearbox.net> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20240913191754.13290-1-daniel@iogearbox.net> References: <20240913191754.13290-1-daniel@iogearbox.net> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.103.10/27397/Fri Sep 13 10:48:01 2024) X-Patchwork-Delegate: bpf@iogearbox.net For all non-tracing helpers which formerly had ARG_PTR_TO_{LONG,INT} as input arguments, zero the value for the case of an error as otherwise it could leak memory. For tracing, it is not needed given CAP_PERFMON can already read all kernel memory anyway hence bpf_get_func_arg() and bpf_get_func_ret() is skipped in here. Also, the MTU helpers mtu_len pointer value is being written but also read. Technically, the MEM_UNINIT should not be there in order to always force init. Removing MEM_UNINIT needs more verifier rework though: MEM_UNINIT right now implies two things actually: i) write into memory, ii) memory does not have to be initialized. If we lift MEM_UNINIT, it then becomes: i) read into memory, ii) memory must be initialized. This means that for bpf_*_check_mtu() we're readding the issue we're trying to fix, that is, it would then be able to write back into things like .rodata BPF maps. Follow-up work will rework the MEM_UNINIT semantics such that the intent can be better expressed. For now just clear the *mtu_len on error path which can be lifted later again. Fixes: 8a67f2de9b1d ("bpf: expose bpf_strtol and bpf_strtoul to all program types") Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers") Signed-off-by: Daniel Borkmann Link: https://lore.kernel.org/bpf/e5edd241-59e7-5e39-0ee5-a51e31b6840a@iogearbox.net --- v1 -> v2: - only set *mtu_len in error path (Alexei) v4 -> v5: - extend explanation on MEM_UNINIT kernel/bpf/helpers.c | 2 ++ kernel/bpf/syscall.c | 1 + net/core/filter.c | 44 +++++++++++++++++++++++--------------------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 5e97fdc6b20f..1a43d06eab28 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -523,6 +523,7 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags, long long _res; int err; + *res = 0; err = __bpf_strtoll(buf, buf_len, flags, &_res); if (err < 0) return err; @@ -548,6 +549,7 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags, bool is_negative; int err; + *res = 0; err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative); if (err < 0) return err; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index a7cd3719e26a..593822d3d393 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -5934,6 +5934,7 @@ static const struct bpf_func_proto bpf_sys_close_proto = { BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res) { + *res = 0; if (flags) return -EINVAL; diff --git a/net/core/filter.c b/net/core/filter.c index d0550c87e520..e4a4454df5f9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6262,20 +6262,25 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb, int ret = BPF_MTU_CHK_RET_FRAG_NEEDED; struct net_device *dev = skb->dev; int skb_len, dev_len; - int mtu; + int mtu = 0; - if (unlikely(flags & ~(BPF_MTU_CHK_SEGS))) - return -EINVAL; + if (unlikely(flags & ~(BPF_MTU_CHK_SEGS))) { + ret = -EINVAL; + goto out; + } - if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len))) - return -EINVAL; + if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len))) { + ret = -EINVAL; + goto out; + } dev = __dev_via_ifindex(dev, ifindex); - if (unlikely(!dev)) - return -ENODEV; + if (unlikely(!dev)) { + ret = -ENODEV; + goto out; + } mtu = READ_ONCE(dev->mtu); - dev_len = mtu + dev->hard_header_len; /* If set use *mtu_len as input, L3 as iph->tot_len (like fib_lookup) */ @@ -6293,15 +6298,12 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb, */ if (skb_is_gso(skb)) { ret = BPF_MTU_CHK_RET_SUCCESS; - if (flags & BPF_MTU_CHK_SEGS && !skb_gso_validate_network_len(skb, mtu)) ret = BPF_MTU_CHK_RET_SEGS_TOOBIG; } out: - /* BPF verifier guarantees valid pointer */ *mtu_len = mtu; - return ret; } @@ -6311,19 +6313,21 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp, struct net_device *dev = xdp->rxq->dev; int xdp_len = xdp->data_end - xdp->data; int ret = BPF_MTU_CHK_RET_SUCCESS; - int mtu, dev_len; + int mtu = 0, dev_len; /* XDP variant doesn't support multi-buffer segment check (yet) */ - if (unlikely(flags)) - return -EINVAL; + if (unlikely(flags)) { + ret = -EINVAL; + goto out; + } dev = __dev_via_ifindex(dev, ifindex); - if (unlikely(!dev)) - return -ENODEV; + if (unlikely(!dev)) { + ret = -ENODEV; + goto out; + } mtu = READ_ONCE(dev->mtu); - - /* Add L2-header as dev MTU is L3 size */ dev_len = mtu + dev->hard_header_len; /* Use *mtu_len as input, L3 as iph->tot_len (like fib_lookup) */ @@ -6333,10 +6337,8 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp, xdp_len += len_diff; /* minus result pass check */ if (xdp_len > dev_len) ret = BPF_MTU_CHK_RET_FRAG_NEEDED; - - /* BPF verifier guarantees valid pointer */ +out: *mtu_len = mtu; - return ret; }