Received-SPF: pass (google.com: domain of linux-kernel+bounces-69296-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Arnd Bergmann <arnd@kernel.org>
To: Steffen Klassert <steffen.klassert@secunet.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>,
	Nathan Chancellor <nathan@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Leon Romanovsky <leon@kernel.org>,
	Lin Ma <linma@zju.edu.cn>,
	Simon Horman <horms@kernel.org>,
	Breno Leitao <leitao@debian.org>,
	Tobias Brunner <tobias@strongswan.org>,
	Raed Salem <raeds@nvidia.com>,
	netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev
Subject: [PATCH] [RFC] xfrm: work around a clang-19 fortifiy-string false-positive
Date: Fri, 16 Feb 2024 21:26:40 +0100
Message-Id: <20240216202657.2493685-1-arnd@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Arnd Bergmann <arnd@arndb.de>

clang-19 recently got branched from clang-18 and is not yet released.
The current version introduces exactly one new warning that I came
across in randconfig testing, in the copy_to_user_tmpl() function:

include/linux/fortify-string.h:420:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
  420 |                         __write_overflow_field(p_size_field, size);

I have not yet produced a minimized test case for it, but I have a
local workaround, which avoids the memset() here by replacing it with
an initializer.

The memset is required to avoid leaking stack data to user space
and was added in commit 1f86840f8977 ("xfrm_user: fix info leak in
copy_to_user_tmpl()"). Simply changing the initializer to set all fields
still risks leaking data in the padding between them, which the compiler
is free to do here. To work around that problem, explicit padding fields
have to get added as well.

My first idea was that just adding the padding would avoid the warning
as well, as the padding tends to confused the fortified string helpers,
but it turns out that both changes are required here.

Since this is a false positive, a better fix would likely be to
fix the compiler.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/uapi/linux/xfrm.h | 3 +++
 net/xfrm/xfrm_user.c      | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 6a77328be114..99adac4fa648 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -27,6 +27,7 @@ struct xfrm_id {
 	xfrm_address_t	daddr;
 	__be32		spi;
 	__u8		proto;
+	__u8		__pad[3];
 };
 
 struct xfrm_sec_ctx {
@@ -242,11 +243,13 @@ struct xfrm_user_sec_ctx {
 struct xfrm_user_tmpl {
 	struct xfrm_id		id;
 	__u16			family;
+	__u16			__pad1;
 	xfrm_address_t		saddr;
 	__u32			reqid;
 	__u8			mode;
 	__u8			share;
 	__u8			optional;
+	__u8			__pad2;
 	__u32			aalgos;
 	__u32			ealgos;
 	__u32			calgos;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a5232dcfea46..e81f977e183c 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2011,7 +2011,7 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 static int copy_to_user_tmpl(struct xfrm_policy *xp, struct sk_buff *skb)
 {
-	struct xfrm_user_tmpl vec[XFRM_MAX_DEPTH];
+	struct xfrm_user_tmpl vec[XFRM_MAX_DEPTH] = {};
 	int i;
 
 	if (xp->xfrm_nr == 0)
@@ -2021,7 +2021,6 @@ static int copy_to_user_tmpl(struct xfrm_policy *xp, struct sk_buff *skb)
 		struct xfrm_user_tmpl *up = &vec[i];
 		struct xfrm_tmpl *kp = &xp->xfrm_vec[i];
 
-		memset(up, 0, sizeof(*up));
 		memcpy(&up->id, &kp->id, sizeof(up->id));
 		up->family = kp->encap_family;
 		memcpy(&up->saddr, &kp->saddr, sizeof(up->saddr));
-- 
2.39.2