Skip to contentSkip to conversation

net: dev: Convert sa_data to flexible array in struct sockaddr

PATCH Applied Kees Cook 3 participants 4 messages Oct 18, 2022 – Oct 25, 2022

Subsystems (4)
Networking Network drivers Sockets Packet sockets
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 net: dev: Convert sa_data to flexible array in struct sockaddr

One of the worst offenders of "fake flexible arrays" is struct sockaddr, as it is the classic example of why GCC and Clang have been traditionally forced to treat all trailing arrays as fake flexible arrays: in the distant misty past, sa_data became too small, and code started just treating it as a flexible array, even though it was fixed-size. The special case by the compiler is specifically that sizeof(sa->sa_data) and FORTIFY_SOURCE (which uses __builtin_object_size(sa->sa_data, 1)) do not agree (14 and -1 respectively), which makes FORTIFY_SOURCE treat it as a flexible array.

However, the coming -fstrict-flex-arrays compiler flag will remove these special cases so that FORTIFY_SOURCE can gain coverage over all the trailing arrays in the kernel that are _not_ supposed to be treated as a flexible array. To deal with this change, convert sa_data to a true flexible array. To keep the structure size the same, move sa_data into a union with a newly introduced sa_data_min with the original size. The result is that FORTIFY_SOURCE can continue to have no idea how large sa_data may actually be, but anything using sizeof(sa->sa_data) must switch to sizeof(sa->sa_data_min).

Cc Jakub Kicinski <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Pavel Begunkov <[email protected]>
Cc David Ahern <[email protected]>
Cc Dylan Yudaken <[email protected]>
Cc Yajun Deng <[email protected]>
Cc Petr Machata <[email protected]>
Cc Hangbin Liu <[email protected]>
Cc Leon Romanovsky <[email protected]>
Cc syzbot <[email protected]>
Cc Willem de Bruijn <[email protected]>
Cc Pablo Neira Ayuso <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 include/linux/socket.h |  5 ++++-
 net/core/dev.c         |  2 +-
 net/core/dev_ioctl.c   |  2 +-
 net/packet/af_packet.c | 10 +++++-----
 4 files changed, 11 insertions(+), 8 deletions(-)
include/linux/socket.h +4 -1
index de3701a2a212..13c3a237b9c9 100644
@@ -33,7 +33,10 @@ typedef __kernel_sa_family_t	sa_family_t;
 
 struct sockaddr {
 	sa_family_t	sa_family;	/* address family, AF_xxx	*/
-	char		sa_data[14];	/* 14 bytes of protocol address	*/
+	union {
+		char sa_data_min[14];		/* Minimum 14 bytes of protocol address	*/
+		DECLARE_FLEX_ARRAY(char, sa_data);
+	};
 };
 
 struct linger {
net/core/dev.c +1 -1
index fa53830d0683..0649826e5803 100644
@@ -8818,7 +8818,7 @@ EXPORT_SYMBOL(dev_set_mac_address_user);
 
 int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name)
 {
-	size_t size = sizeof(sa->sa_data);
+	size_t size = sizeof(sa->sa_data_min);
 	struct net_device *dev;
 	int ret = 0;
net/core/dev_ioctl.c +1 -1
index 7674bb9f3076..5cdbfbf9a7dc 100644
@@ -342,7 +342,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 		if (ifr->ifr_hwaddr.sa_family != dev->type)
 			return -EINVAL;
 		memcpy(dev->broadcast, ifr->ifr_hwaddr.sa_data,
-		       min(sizeof(ifr->ifr_hwaddr.sa_data),
+		       min(sizeof(ifr->ifr_hwaddr.sa_data_min),
 			   (size_t)dev->addr_len));
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 		return 0;
net/packet/af_packet.c +5 -5
index 6ce8dd19f33c..8c5b3da0c29f 100644
@@ -3277,7 +3277,7 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 			    int addr_len)
 {
 	struct sock *sk = sock->sk;
-	char name[sizeof(uaddr->sa_data) + 1];
+	char name[sizeof(uaddr->sa_data_min) + 1];
 
 	/*
 	 *	Check legality

@@ -3288,8 +3288,8 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 	/* uaddr->sa_data comes from the userspace, it's not guaranteed to be
 	 * zero-terminated.
 	 */
-	memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data));
-	name[sizeof(uaddr->sa_data)] = 0;
+	memcpy(name, uaddr->sa_data, sizeof(uaddr->sa_data_min));
+	name[sizeof(uaddr->sa_data_min)] = 0;
 
 	return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
 }

@@ -3561,11 +3561,11 @@ static int packet_getname_spkt(struct socket *sock, struct sockaddr *uaddr,
 		return -EOPNOTSUPP;
 
 	uaddr->sa_family = AF_PACKET;
-	memset(uaddr->sa_data, 0, sizeof(uaddr->sa_data));
+	memset(uaddr->sa_data, 0, sizeof(uaddr->sa_data_min));
 	rcu_read_lock();
 	dev = dev_get_by_index_rcu(sock_net(sk), READ_ONCE(pkt_sk(sk)->ifindex));
 	if (dev)
-		strscpy(uaddr->sa_data, dev->name, sizeof(uaddr->sa_data));
+		strscpy(uaddr->sa_data, dev->name, sizeof(uaddr->sa_data_min));
 	rcu_read_unlock();
 
 	return sizeof(*uaddr);
Signature · 1 line
2.34.1
maintainer
#1 lore raw

Hello,

Quoted 49 lines · Kees Cook

One of the worst offenders of "fake flexible arrays" is struct sockaddr, as it is the classic example of why GCC and Clang have been traditionally forced to treat all trailing arrays as fake flexible arrays: in the distant misty past, sa_data became too small, and code started just treating it as a flexible array, even though it was fixed-size. The special case by the compiler is specifically that sizeof(sa->sa_data) and FORTIFY_SOURCE (which uses __builtin_object_size(sa->sa_data, 1)) do not agree (14 and -1 respectively), which makes FORTIFY_SOURCE treat it as a flexible array.

However, the coming -fstrict-flex-arrays compiler flag will remove these special cases so that FORTIFY_SOURCE can gain coverage over all the trailing arrays in the kernel that are _not_ supposed to be treated as a flexible array. To deal with this change, convert sa_data to a true flexible array. To keep the structure size the same, move sa_data into a union with a newly introduced sa_data_min with the original size. The result is that FORTIFY_SOURCE can continue to have no idea how large sa_data may actually be, but anything using sizeof(sa->sa_data) must switch to sizeof(sa->sa_data_min).

Cc Jakub Kicinski <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Pavel Begunkov <[email protected]>
Cc David Ahern <[email protected]>
Cc Dylan Yudaken <[email protected]>
Cc Yajun Deng <[email protected]>
Cc Petr Machata <[email protected]>
Cc Hangbin Liu <[email protected]>
Cc Leon Romanovsky <[email protected]>
Cc syzbot <[email protected]>
Cc Willem de Bruijn <[email protected]>
Cc Pablo Neira Ayuso <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 include/linux/socket.h |  5 ++++-
 net/core/dev.c         |  2 +-
 net/core/dev_ioctl.c   |  2 +-
 net/packet/af_packet.c | 10 +++++-----
 4 files changed, 11 insertions(+), 8 deletions(-)
include/linux/socket.h +0 -0
index de3701a2a212..13c3a237b9c9 100644

include/linux/socket.h typedef __kernel_sa_family_t sa_family_t; · line 38

Quoted 7 lines
include/linux/socket.h +3 -1
@@ -33,7 +33,10 @@ typedef __kernel_sa_family_t	sa_family_t;
 
 struct sockaddr {
 	sa_family_t	sa_family;	/* address family, AF_xxx	*/
-	char		sa_data[14];	/* 14 bytes of protocol address	*/
+	union {
+		char sa_data_min[14];		/* Minimum 14 bytes of protocol address	*/
+		DECLARE_FLEX_ARRAY(char, sa_data);

Any special reason to avoid preserving the old name for the array and e.g. using sa_data_flex for the new field, so we don't have to touch the sockaddr users?

Thanks!

Paolo

author
#2 lore raw
Quoted 12 lines · Paolo Abeni

include/linux/socket.h typedef __kernel_sa_family_t sa_family_t; · line 38

Quoted 7 lines · Kees Cook

[...]
struct sockaddr {

include/linux/socket.h +3 -1
 	sa_family_t	sa_family;	/* address family, AF_xxx	*/
-	char		sa_data[14];	/* 14 bytes of protocol address	*/
+	union {
+		char sa_data_min[14];		/* Minimum 14 bytes of protocol address	*/
+		DECLARE_FLEX_ARRAY(char, sa_data);

Any special reason to avoid preserving the old name for the array and e.g. using sa_data_flex for the new field, so we don't have to touch the sockaddr users?

Yes -- the reason is exactly to not touch the sockaddr users (who generally treat sa_data as a fake flexible array). By switching it to a flex-array the behavior will stay the same (especially under the coming -fstrict-flex-arrays option), except that it breaks sizeof(). But the broken sizeof() allows us to immediately find all the places where the code explicitly depends on sa_data being 14 bytes. And for those cases, we switch to sizeof(sa_data_min).

If we went the reverse route (and added -fstrict-flex-arrays) we might end up adding a bunch of false positives all at once, because the places that treated it as a flex-array would suddenly all begin behaving as a 14-byte array.

Signature · 1 line
Kees Cook
CI Applied
#3 lore raw
In reply to Kees Cook
This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Quoted 11 lines · On Tue, 18 Oct 2022 02:56:03 -0700 you wrote:

One of the worst offenders of "fake flexible arrays" is struct sockaddr, as it is the classic example of why GCC and Clang have been traditionally forced to treat all trailing arrays as fake flexible arrays: in the distant misty past, sa_data became too small, and code started just treating it as a flexible array, even though it was fixed-size. The special case by the compiler is specifically that sizeof(sa->sa_data) and FORTIFY_SOURCE (which uses __builtin_object_size(sa->sa_data, 1)) do not agree (14 and -1 respectively), which makes FORTIFY_SOURCE treat it as a flexible array.

[...]

Here is the summary with links:
- [next] net: dev: Convert sa_data to flexible array in struct sockaddr

    https://git.kernel.org/netdev/net-next/c/b5f0de6df6dc

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html