diff mbox series

[RFC] xfrm: work around a clang-19 fortifiy-string false-positive

Message ID 20240216202657.2493685-1-arnd@kernel.org
State New
Headers
Series [RFC] xfrm: work around a clang-19 fortifiy-string false-positive |

Commit Message

Arnd Bergmann Feb. 16, 2024, 8:26 p.m. UTC 
  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(-)

Comments

Nathan Chancellor Feb. 16, 2024, 8:42 p.m. UTC | #1 
Hi Arnd,

On Fri, Feb 16, 2024 at 09:26:40PM +0100, Arnd Bergmann wrote:
> 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.

I have some observations and notes from my initial investigation into
this issue on our GitHub issue tracker but I have not produced a
minimized test case either.

https://github.com/ClangBuiltLinux/linux/issues/1985

> 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
>
Kees Cook Feb. 16, 2024, 9:19 p.m. UTC | #2 
On Fri, Feb 16, 2024 at 09:26:40PM +0100, Arnd Bergmann wrote:
> 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.

Per C11, padding bits are zero initialized if there is an initializer,
so "= { }" here should be sufficient -- no need to add the struct
members.

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

As Nathan has found, this appears to be a loop unrolling bug in Clang.
https://github.com/ClangBuiltLinux/linux/issues/1985

The shorter fix (in the issue) is to explicitly range-check before
the loop:

       if (xp->xfrm_nr > XFRM_MAX_DEPTH)
               return -ENOBUFS;
diff mbox series

Patch

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));