Skip to contentSkip to conversation

packet: validate address length if non-zero

PATCH Applied Willem de Bruijn 4 participants 19 messages Dec 22, 2018 – Apr 26, 2019

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

Willem de Bruijn

author maintainer
#0 lore raw

PATCH 1/1 packet: validate address length if non-zero

From: Willem de Bruijn <[email protected]>

Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -2
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 						sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		addr	= saddr->sll_addr;
+		addr	= saddr->sll_halen ? saddr->sll_addr : NULL;
 		dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
 		if (addr && dev && saddr->sll_halen < dev->addr_len)
 			goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		addr	= saddr->sll_addr;
+		addr	= saddr->sll_halen ? saddr->sll_addr : NULL;
 		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
 		if (addr && dev && saddr->sll_halen < dev->addr_len)
 			goto out;
Signature · 1 line
2.20.1.415.g653613c723-goog
maintainer Applied
#1 lore raw

From: Willem de Bruijn <[email protected]>
Date: Sat, 22 Dec 2018 16:53:45 -0500

Quoted 8 lines

From: Willem de Bruijn <[email protected]>

Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>

Applied, thanks Willem.

I'll put this into -stable next to the patch it fixes.

Thanks again.

author maintainer
#2 lore raw
Quoted 9 lines · David Miller

From: Willem de Bruijn <[email protected]>
Date: Sat, 22 Dec 2018 16:53:45 -0500

Quoted 5 lines

From: Willem de Bruijn <[email protected]>

Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Quoted 7 lines
Quoted 3 lines
Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>

Applied, thanks Willem.

I'll put this into -stable next to the patch it fixes.

Thanks David. Sorry for the earlier cock-up.

Looking at this, I noticed one header_ops.create implementation that always expects non-zero daddr. Will send a small fix for that, too.

#4 lore raw
In reply to Willem de Bruijn

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 						sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		addr	= saddr->sll_addr;
+		addr	= saddr->sll_halen ? saddr->sll_addr : NULL;
 		dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
 		if (addr && dev && saddr->sll_halen < dev->addr_len)
 			goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		addr	= saddr->sll_addr;
+		addr	= saddr->sll_halen ? saddr->sll_addr : NULL;
 		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
 		if (addr && dev && saddr->sll_halen < dev->addr_len)
 			goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#5 lore raw
Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

#6 lore raw

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

		addr    = saddr->sll_addr;
		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
		if (dev && msg->msg_namelen < (dev->addr_len
					+ offsetof(struct sockaddr_ll,
						sll_addr)))
			/* Don't read address from beyond the end of the buffer */
			goto out;

So it just checks that all of the address is in the buffer passed from the user.

Although I'd write those tests in a different order:

		if (dev && offsetof(struct sockaddr_ll, sll_addr) + dev->addr_len > msg->msg_namelen)
			goto out;

FWIW the relevant application code has been in our cvs tree for 13 years. Mark Brown might remember where he stole it from :-)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#7 lore raw
Quoted 88 lines · David Laight

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Although I'd write those tests in a different order:

                if (dev && offsetof(struct sockaddr_ll, sll_addr) + dev->addr_len > msg->msg_namelen)
                        goto out;

Since there already is a test for msg->msg_namelen < sizeof(struct sockaddr_ll) just before, I'd keep the same order.

Do you want to send a fix, or shall I?

author maintainer
#8 lore raw
Quoted 90 lines · Willem de Bruijn
Quoted 88 lines · David Laight

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

Quoted 9 lines
Quoted 4 lines

Although I'd write those tests in a different order:

                if (dev && offsetof(struct sockaddr_ll, sll_addr) + dev->addr_len > msg->msg_namelen)
                        goto out;

Since there already is a test for msg->msg_namelen < sizeof(struct sockaddr_ll) just before, I'd keep the same order.

Do you want to send a fix, or shall I?

author maintainer
#9 lore raw
Quoted 102 lines · Willem de Bruijn
Quoted 90 lines · Willem de Bruijn
Quoted 88 lines · David Laight

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

So something like

net/packet/af_packet.c +0 -0
index 5c4a118d6f969..64ab3c960f538 100644
@@ -2819,12 +2819,10 @@ static int packet_snd(struct socket *sock,

struct msghdr *msg, size_t len)

                err = -EINVAL;
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
-               if (msg->msg_namelen < (saddr->sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
-                       goto out;
                proto   = saddr->sll_protocol;
-               addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
+               addr    = sock->type == SOCK_DGRAM ? saddr->sll_addr : NULL;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-               if (addr && dev && saddr->sll_halen < dev->addr_len)
+               if (addr && dev && msg->msg_namelen < (dev->addr_len +
offsetof(struct sockaddr_ll, sll_addr)))
                        goto out_unlock;
author maintainer
#10 lore raw
Quoted 130 lines · Willem de Bruijn
Quoted 102 lines · Willem de Bruijn
Quoted 90 lines · Willem de Bruijn
Quoted 88 lines · David Laight

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

So something like

net/packet/af_packet.c +2 -4
index 5c4a118d6f969..64ab3c960f538 100644
@@ -2819,12 +2819,10 @@ static int packet_snd(struct socket *sock,
 struct msghdr *msg, size_t len)
                err = -EINVAL;
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
-               if (msg->msg_namelen < (saddr->sll_halen +
 offsetof(struct sockaddr_ll, sll_addr)))
-                       goto out;
                proto   = saddr->sll_protocol;
-               addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
+               addr    = sock->type == SOCK_DGRAM ? saddr->sll_addr : NULL;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-               if (addr && dev && saddr->sll_halen < dev->addr_len)
+               if (addr && dev && msg->msg_namelen < (dev->addr_len +
 offsetof(struct sockaddr_ll, sll_addr)))
Quoted 1 line
                        goto out_unlock;

Sent http://patchwork.ozlabs.org/patch/1090340/

Though I probably misunderstood your issue.

I initially thought that an sll_halen > 0 < dev->addr_len was triggering the immediately check.

But I guess that the real issue was an sll_halen == 0 was causing addr to be NULL, but dev_hard_header still called. For Ethernet, eth_header then does not fail, but simply does not fill in eth->h_dest.

Let me know if you'd prefer a revised commit message.

author maintainer
#11 lore raw
Quoted 137 lines · Willem de Bruijn
Quoted 130 lines · Willem de Bruijn
Quoted 102 lines · Willem de Bruijn
Quoted 90 lines · Willem de Bruijn
Quoted 88 lines · David Laight

From: Willem de Bruijn

Quoted 69 lines

Sent: 23 April 2019 16:08

Quoted 41 lines · David Laight

From: Willem de Bruijn

Quoted 34 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -3
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--
Quoted 1 line

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

So something like

net/packet/af_packet.c +2 -4
index 5c4a118d6f969..64ab3c960f538 100644
@@ -2819,12 +2819,10 @@ static int packet_snd(struct socket *sock,
 struct msghdr *msg, size_t len)
                err = -EINVAL;
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
-               if (msg->msg_namelen < (saddr->sll_halen +
 offsetof(struct sockaddr_ll, sll_addr)))
-                       goto out;
                proto   = saddr->sll_protocol;
-               addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
+               addr    = sock->type == SOCK_DGRAM ? saddr->sll_addr : NULL;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-               if (addr && dev && saddr->sll_halen < dev->addr_len)
+               if (addr && dev && msg->msg_namelen < (dev->addr_len +
 offsetof(struct sockaddr_ll, sll_addr)))
Quoted 1 line
                        goto out_unlock;

Sent http://patchwork.ozlabs.org/patch/1090340/

Quoted 8 lines

Though I probably misunderstood your issue.

I initially thought that an sll_halen > 0 < dev->addr_len was triggering the immediately check.

But I guess that the real issue was an sll_halen == 0 was causing addr to be NULL, but dev_hard_header still called. For Ethernet, eth_header then does not fail, but simply does not fill in eth->h_dest.

Luckily I misread. It *does* fail and so does sendto. But indeed in a different way than I previously understood your report.

Quoted 1 line

Let me know if you'd prefer a revised commit message.

This, then, still stands.

#12 lore raw

From: Willem de Bruijn

Quoted 148 lines

Sent: 24 April 2019 20:35

Quoted 28 lines · Willem de Bruijn
Quoted 25 lines · Willem de Bruijn
Quoted 22 lines · Willem de Bruijn
Quoted 19 lines · Willem de Bruijn
Quoted 17 lines · David Laight

From: Willem de Bruijn

Quoted 15 lines

Sent: 23 April 2019 16:08

Quoted 13 lines · David Laight

From: Willem de Bruijn

Quoted 11 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

net/packet/af_packet.c static int tpacket_snd(struct packet_sock *po, struct msghdr

Quoted 5 lines
net/packet/af_packet.c +0 -0
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr

*msg)

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg,

Quoted 9 lines
net/packet/af_packet.c +1 -1
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg,

size_t len)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg, · line 2825

Quoted 1 line
net/packet/af_packet.c +0 -0
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll,

sll_addr)))

Quoted 99 lines
Quoted 90 lines
Quoted 65 lines
Quoted 56 lines
Quoted 56 lines
Quoted 39 lines
Quoted 13 lines
Quoted 9 lines
net/packet/af_packet.c +1 -2
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

So something like

net/packet/af_packet.c +2 -4
index 5c4a118d6f969..64ab3c960f538 100644
@@ -2819,12 +2819,10 @@ static int packet_snd(struct socket *sock,
 struct msghdr *msg, size_t len)
                err = -EINVAL;
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
-               if (msg->msg_namelen < (saddr->sll_halen +
 offsetof(struct sockaddr_ll, sll_addr)))
-                       goto out;
                proto   = saddr->sll_protocol;
-               addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
+               addr    = sock->type == SOCK_DGRAM ? saddr->sll_addr : NULL;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-               if (addr && dev && saddr->sll_halen < dev->addr_len)
+               if (addr && dev && msg->msg_namelen < (dev->addr_len +
 offsetof(struct sockaddr_ll, sll_addr)))
Quoted 1 line
                        goto out_unlock;

Sent http://patchwork.ozlabs.org/patch/1090340/

Though I probably misunderstood your issue.

I initially thought that an sll_halen > 0 < dev->addr_len was triggering the immediately check.

Quoted 6 lines
Quoted 3 lines

But I guess that the real issue was an sll_halen == 0 was causing addr to be NULL, but dev_hard_header still called. For Ethernet, eth_header then does not fail, but simply does not fill in eth->h_dest.

Luckily I misread. It *does* fail and so does sendto. But indeed in a different way than I previously understood your report.

I didn't track down where the EINVAL came from.

Quoted 1 line
Quoted 1 line

Let me know if you'd prefer a revised commit message.

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Looking at it all again how about:

	char *addr = NULL;
	...
			err = -EINVAL;
			if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
				goto out;
			proto = saddr->sll_protocol;
			dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
			if (dev && sock->type == SOCK_DGRAM) {
				if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
					goto out_unlock;
				addr = saddr->sll_addr;
			}

Although it might even be worth moving the check for (dev == NULL) inside the conditional.

Then we'll paint the bikeshed in Paisley.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#13 lore raw
Quoted 166 lines · David Laight

From: Willem de Bruijn

Quoted 148 lines

Sent: 24 April 2019 20:35

Quoted 28 lines · Willem de Bruijn
Quoted 25 lines · Willem de Bruijn
Quoted 22 lines · Willem de Bruijn
Quoted 19 lines · Willem de Bruijn
Quoted 17 lines · David Laight

From: Willem de Bruijn

Quoted 15 lines

Sent: 23 April 2019 16:08

Quoted 13 lines · David Laight

From: Willem de Bruijn

Quoted 11 lines

Sent: 22 December 2018 21:54
Validate packet socket address length if a length is given. Zero length is equivalent to not setting an address.

Fixes 99137b7888f4 ("packet: validate address length")
Reported-by Ido Schimmel <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

net/packet/af_packet.c static int tpacket_snd(struct packet_sock *po, struct msghdr

Quoted 5 lines
net/packet/af_packet.c +0 -0
index 5dda263b4a0a..eedacdebcd4c 100644
@@ -2625,7 +2625,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr

*msg)

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg,

Quoted 9 lines
net/packet/af_packet.c +1 -1
                                              sll_addr)))
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg,

size_t len)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

net/packet/af_packet.c static int packet_snd(struct socket *sock, struct msghdr *msg, · line 2825

Quoted 1 line
net/packet/af_packet.c +0 -0
              if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll,

sll_addr)))

Quoted 99 lines
Quoted 90 lines
Quoted 65 lines
Quoted 56 lines
Quoted 56 lines
Quoted 39 lines
Quoted 13 lines
Quoted 9 lines
net/packet/af_packet.c +1 -2
                      goto out;
              proto   = saddr->sll_protocol;
-             addr    = saddr->sll_addr;
+             addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
              dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
              if (addr && dev && saddr->sll_halen < dev->addr_len)
                      goto out;
--

2.20.1.415.g653613c723-goog

We've just discovered the combination of this patch and the one it 'fixes' breaks some of our userspace code.

Quoted 7 lines

Prior to these changes it didn't matter if code using AF_PACKET to send ethernet frames on a specific 'ethertype' failed to set sll_addr. Everything assumed it would be 6 - and the packets were sent.

With both changes you get a -EINVAL return from somewhere. I can fix our code, but I doubt it is the only code affected. Other people are likely to have copied the same example.

Thanks for the report.

Usage trumps correctness. But this seems to be a case of damned if you do, damned if you don't.

Syzbot found a real use case of reading beyond the end of msg->msg_namelen, since that is checked against

                if (msg->msg_namelen < (saddr->sll_halen +

offsetof(struct sockaddr_ll, sll_addr)))

Just assuming that address length is dev->addr_len allows an ns_capable root to build link layer packets with address set to uninitialized data.

Ethernet is not the most problematic link layer. Indeed, since ETH_ALEN < sizeof(sll_addr), the previous check

Quoted 7 lines
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))

Will be sufficient in this case. The syzbot report was on a device of type ip6gre, with addr_len sizeof(struct in6_addr).

So I can refine to only perform the check on protocols with addr_len

Quoted 1 line

= sizeof(sll_addr), excluding Ethernet.

Maybe something like:

                addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                if (dev && msg->msg_namelen < (dev->addr_len
                                        + offsetof(struct sockaddr_ll,
                                                sll_addr)))
                        /* Don't read address from beyond the end of the buffer */
                        goto out;
Quoted 8 lines
Quoted 2 lines

So it just checks that all of the address is in the buffer passed from the user.

Yes. sll_halen is never used outside this block. Testing that against dev->addr_len just adds needless a level of indirection. We only care that code that assumes the address is dev->addr_len won't read beyond the end of msg->namelen. So this looks great to me (aside from goto out_unlock). Thanks.

Quoted 3 lines

Actually, this only matters if sll_addr may be read, which is only true for SOCK_DGRAM. It is fine to pass a sockaddr_ll without an address for SOCK_RAW.

So something like

net/packet/af_packet.c +2 -4
index 5c4a118d6f969..64ab3c960f538 100644
@@ -2819,12 +2819,10 @@ static int packet_snd(struct socket *sock,
 struct msghdr *msg, size_t len)
                err = -EINVAL;
                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
-               if (msg->msg_namelen < (saddr->sll_halen +
 offsetof(struct sockaddr_ll, sll_addr)))
-                       goto out;
                proto   = saddr->sll_protocol;
-               addr    = saddr->sll_halen ? saddr->sll_addr : NULL;
+               addr    = sock->type == SOCK_DGRAM ? saddr->sll_addr : NULL;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-               if (addr && dev && saddr->sll_halen < dev->addr_len)
+               if (addr && dev && msg->msg_namelen < (dev->addr_len +
 offsetof(struct sockaddr_ll, sll_addr)))
Quoted 1 line
                        goto out_unlock;

Sent http://patchwork.ozlabs.org/patch/1090340/

Though I probably misunderstood your issue.

I initially thought that an sll_halen > 0 < dev->addr_len was triggering the immediately check.

Quoted 6 lines
Quoted 3 lines

But I guess that the real issue was an sll_halen == 0 was causing addr to be NULL, but dev_hard_header still called. For Ethernet, eth_header then does not fail, but simply does not fill in eth->h_dest.

Luckily I misread. It *does* fail and so does sendto. But indeed in a different way than I previously understood your report.

I didn't track down where the EINVAL came from.

Quoted 1 line
Quoted 1 line

Let me know if you'd prefer a revised commit message.

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

Quoted 6 lines

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Quoted 13 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }

Yes, given the above, this looks great to me.

In general I'm hesitant to loosen interface constraints. As you can never tighten them again. But given the change on recv, it seems correct here. That is technically a separate issue, so worth a separate patch, I think. If that's not too pedantic. Else at least an extra Fixes tag.

Quoted 4 lines

Although it might even be worth moving the check for (dev == NULL) inside the conditional.

Then we'll paint the bikeshed in Paisley.

:)

Agreed on moving the dev. That makes it really obvious that addr is only used in datagram mode. And indeed exactly matches the branch around the only user, dev_hard_header.

Quoted 5 lines

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

#14 lore raw

From: Willem de Bruijn

Quoted 1 line

Sent: 25 April 2019 14:57

...

Quoted 12 lines
Quoted 11 lines

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Quoted 2 lines

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Fortunately I didn't have to find the pre-git sources :-)

Quoted 14 lines
Quoted 13 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }
Quoted 4 lines

Yes, given the above, this looks great to me.

In general I'm hesitant to loosen interface constraints. As you can never tighten them again.

Indeed.

Quoted 4 lines

But given the change on recv, it seems
correct here. That is technically a separate issue, so worth a separate patch, I think. If that's not too pedantic. Else at least an extra Fixes tag.

Or leave the above using 'sizeof' and change the receive code to pad the address to sizeof struct sockaddr_ll.
Which is definitely a completely different fix.
The rx code seems to be:

		if (sock->type == SOCK_PACKET) {
			__sockaddr_check_size(sizeof(struct sockaddr_pkt));
			msg->msg_namelen = sizeof(struct sockaddr_pkt);
		} else {
			struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;

			msg->msg_namelen = sll->sll_halen +
				offsetof(struct sockaddr_ll, sll_addr);
		}
		memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
		       msg->msg_namelen);

Hopefully the buffer is always big enough!
Assuming that the code that sets up the address zaps the last two bytes the SOCK_DGRAM side just needs a max(, sizeof (struct sockaddr_ll)). Zeroing the 8 byte field before the mac address is put into it is cheap (one 64bit write on 64bit systems).

I guess this would have 'Fixes' tag for the 2.6.14 git tag!

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#15 lore raw
Quoted 64 lines · David Laight

From: Willem de Bruijn

Quoted 1 line

Sent: 25 April 2019 14:57

...

Quoted 12 lines
Quoted 11 lines

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Quoted 2 lines

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Fortunately I didn't have to find the pre-git sources :-)

Quoted 14 lines
Quoted 13 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }
Quoted 4 lines

Yes, given the above, this looks great to me.

In general I'm hesitant to loosen interface constraints. As you can never tighten them again.

Indeed.

Quoted 4 lines

But given the change on recv, it seems
correct here. That is technically a separate issue, so worth a separate patch, I think. If that's not too pedantic. Else at least an extra Fixes tag.

Or leave the above using 'sizeof' and change the receive code to pad the address to sizeof struct sockaddr_ll.
Which is definitely a completely different fix.
The rx code seems to be:

                if (sock->type == SOCK_PACKET) {
                        __sockaddr_check_size(sizeof(struct sockaddr_pkt));
                        msg->msg_namelen = sizeof(struct sockaddr_pkt);
                } else {
                        struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;

                        msg->msg_namelen = sll->sll_halen +
                                offsetof(struct sockaddr_ll, sll_addr);
                }
                memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
                       msg->msg_namelen);
Quoted 1 line

Hopefully the buffer is always big enough!

The kernel buffer is always sockaddr_storage with _K_SS_MAXSIZE or 128B. Before copying into skb->cb[] there is the sock_skb_cb_check_size BUILD_BUG_ON based on the assumption that every lladdr len <= MAX_ADDR_LEN (32B). As far as I can telll, that does seem to hold (INFINIBAND_ALEN being the largest at 32). So while there is no explicit check on the memcpy from skb->cb into msg_name, this is safe.

Quoted 6 lines

Assuming that the code that sets up the address zaps the last two bytes the SOCK_DGRAM side just needs a max(, sizeof (struct sockaddr_ll)). Zeroing the 8 byte field before the mac address is put into it is cheap (one 64bit write on 64bit systems).

I guess this would have 'Fixes' tag for the 2.6.14 git tag!

The uaddr_len change is commit 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.")?

That both introduced the recv change to return a length based on dev->addr_len even the total is if below sizeof(struct sockaddr_ll)

- else
- msg->msg_namelen = sizeof(struct sockaddr_ll);
[..]
+ else
+ msg->msg_namelen = sll->sll_halen + offsetof(struct
sockaddr_ll, sll_addr);

and the new sendmsg check

                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;

+ if (msg->msg_namelen < (saddr->sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
+ goto out;

but without removing the preexisting check above it, thus causing the recvfrom () followed by sendto() to fail.

author maintainer
#16 lore raw
Quoted 37 lines · Willem de Bruijn
Quoted 35 lines · David Laight

From: Willem de Bruijn

Quoted 1 line

Sent: 25 April 2019 14:57

...

Quoted 12 lines
Quoted 11 lines

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Quoted 2 lines

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Fortunately I didn't have to find the pre-git sources :-)

Quoted 14 lines
Quoted 13 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct sockaddr_ll, sll_addr))
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }
Quoted 1 line
Quoted 1 line
Quoted 1 line

Yes, given the above, this looks great to me.

Coming back to this. Both the above and two separate send/recv fixes seem fine to me. Do you have a preference either way? And do you want to send the fix(es) or should I?

Thanks,

Willem

#17 lore raw

From: Willem de Bruijn

Quoted 41 lines

Sent: 26 April 2019 16:11

Quoted 23 lines · Willem de Bruijn
Quoted 21 lines · David Laight

From: Willem de Bruijn

Quoted 1 line

Sent: 25 April 2019 14:57

...

Quoted 12 lines
Quoted 11 lines

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Quoted 2 lines

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Fortunately I didn't have to find the pre-git sources :-)

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct

sockaddr_ll, sll_addr))

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

Yes, given the above, this looks great to me.

Coming back to this. Both the above and two separate send/recv fixes seem fine to me. Do you have a preference either way? And do you want to send the fix(es) or should I?

I'll let you do it - save me working out how to get valid patches off my Linux systems and into outlook :-)

If you are going to do the recv fix the send one can keep the check against the full struct sockaddr_ll.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#18 lore raw
Quoted 49 lines · David Laight

From: Willem de Bruijn

Quoted 41 lines

Sent: 26 April 2019 16:11

Quoted 23 lines · Willem de Bruijn
Quoted 21 lines · David Laight

From: Willem de Bruijn

Quoted 1 line

Sent: 25 April 2019 14:57

...

Quoted 12 lines
Quoted 11 lines

I've just done a bit of software archaeology.

Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the receive code. So it is not surprising that old application code leaves it as zero.

The old receive code also always set msg_namelen = sizeof (struct sockaddr_ll). The receive code now sets:
msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
For ethernet this changes the msg_namelen from 20 to 18.
A side effect (no one has noticed for years) is that you can't send a reply by passing back the received address buffer.

Quoted 2 lines

Great find, thanks. I hadn't thought of going back that far, but clearly should in these legacy caller questions..

Fortunately I didn't have to find the pre-git sources :-)

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

Looking at it all again how about:

        char *addr = NULL;
        ...
                        err = -EINVAL;
                        if (msg->msg_namelen < offsetof(struct sockaddr_ll, sll_addr))
                                goto out;
                        proto = saddr->sll_protocol;
                        dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
                        if (dev && sock->type == SOCK_DGRAM) {
                                if (msg->msg_namelen < dev->addr_len + offsetof(struct

sockaddr_ll, sll_addr))

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
                                        goto out_unlock;
                                addr = saddr->sll_addr;
                        }
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

Yes, given the above, this looks great to me.

Coming back to this. Both the above and two separate send/recv fixes seem fine to me. Do you have a preference either way? And do you want to send the fix(es) or should I?

Quoted 5 lines

I'll let you do it - save me working out how to get valid patches off my Linux systems and into outlook :-)

If you are going to do the recv fix the send one can keep the check against the full struct sockaddr_ll.

It took a bit longer than expected, sorry. In the rx one, I went with a branch in the relatively unlikely msg->msg_name branch rather than an unconditional zero in the packet_rcv hot path.

http://patchwork.ozlabs.org/patch/1091764/
http://patchwork.ozlabs.org/patch/1091765/