[PATCH net v2] packet: in recvmsg msg_name return at least sizeof sockaddr_ll

[PATCH net v2] packet: in recvmsg msg_name return at least sizeof sockaddr_ll

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Packet send checks that msg_name is at least sizeof sockaddr_ll.
Packet recv must return at least this length, so that its output
can be passed unmodified to packet send.

This ceased to be true since adding support for lladdr longer than
sll_addr. Since, the return value uses true address length.

Always return at least sizeof sockaddr_ll, even if address length
is shorter. Zero the padding bytes.

Change v1->v2: do not overwrite zeroed padding again. use copy_len.

Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.")
Suggested-by: David Laight <David.Laight@aculab.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e726aaba73b9f..5fe3d75b6212d 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3348,20 +3348,29 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 	sock_recv_ts_and_drops(msg, sk, skb);
 
 	if (msg->msg_name) {
+		int copy_len;
+
 		/* If the address length field is there to be filled
 		 * in, we fill it in now.
 		 */
 		if (sock->type == SOCK_PACKET) {
 			__sockaddr_check_size(sizeof(struct sockaddr_pkt));
 			msg->msg_namelen = sizeof(struct sockaddr_pkt);
+			copy_len = msg->msg_namelen;
 		} else {
 			struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
 
 			msg->msg_namelen = sll->sll_halen +
 				offsetof(struct sockaddr_ll, sll_addr);
+			copy_len = msg->msg_namelen;
+			if (msg->msg_namelen < sizeof(struct sockaddr_ll)) {
+				memset(msg->msg_name +
+				       offsetof(struct sockaddr_ll, sll_addr),
+				       0, sizeof(sll->sll_addr));
+				msg->msg_namelen = sizeof(struct sockaddr_ll);
+			}
 		}
-		memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
-		       msg->msg_namelen);
+		memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);
 	}
 
 	if (pkt_sk(sk)->auxdata) {
-- 
2.21.0.593.g511ec345e18-goog

RE: [PATCH net v2] packet: in recvmsg msg_name return at least sizeof sockaddr_ll

[David Laight]

From: Willem de Bruijn
> Sent: 29 April 2019 16:47
> Packet send checks that msg_name is at least sizeof sockaddr_ll.
> Packet recv must return at least this length, so that its output
> can be passed unmodified to packet send.
> 
> This ceased to be true since adding support for lladdr longer than
> sll_addr. Since, the return value uses true address length.
> 
> Always return at least sizeof sockaddr_ll, even if address length
> is shorter. Zero the padding bytes.
> 
> Change v1->v2: do not overwrite zeroed padding again. use copy_len.
> 
> Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.")
> Suggested-by: David Laight <David.Laight@aculab.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/packet/af_packet.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index e726aaba73b9f..5fe3d75b6212d 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3348,20 +3348,29 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  	sock_recv_ts_and_drops(msg, sk, skb);
> 
>  	if (msg->msg_name) {
> +		int copy_len;
> +
>  		/* If the address length field is there to be filled
>  		 * in, we fill it in now.
>  		 */
>  		if (sock->type == SOCK_PACKET) {
>  			__sockaddr_check_size(sizeof(struct sockaddr_pkt));
>  			msg->msg_namelen = sizeof(struct sockaddr_pkt);
> +			copy_len = msg->msg_namelen;
>  		} else {
>  			struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
> 
>  			msg->msg_namelen = sll->sll_halen +
>  				offsetof(struct sockaddr_ll, sll_addr);
> +			copy_len = msg->msg_namelen;
> +			if (msg->msg_namelen < sizeof(struct sockaddr_ll)) {
> +				memset(msg->msg_name +
> +				       offsetof(struct sockaddr_ll, sll_addr),
> +				       0, sizeof(sll->sll_addr));
> +				msg->msg_namelen = sizeof(struct sockaddr_ll);
> +			}
>  		}
> -		memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
> -		       msg->msg_namelen);
> +		memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);
>  	}
> 
>  	if (pkt_sk(sk)->auxdata) {
> --
> 2.21.0.593.g511ec345e18-goog

Looks ok to me, not tried to compile it though.

	David

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

Re: [PATCH net v2] packet: in recvmsg msg_name return at least sizeof sockaddr_ll

[Willem de Bruijn]

On Mon, Apr 29, 2019 at 11:49 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 29 April 2019 16:47
> > Packet send checks that msg_name is at least sizeof sockaddr_ll.
> > Packet recv must return at least this length, so that its output
> > can be passed unmodified to packet send.
> >
> > This ceased to be true since adding support for lladdr longer than
> > sll_addr. Since, the return value uses true address length.
> >
> > Always return at least sizeof sockaddr_ll, even if address length
> > is shorter. Zero the padding bytes.
> >
> > Change v1->v2: do not overwrite zeroed padding again. use copy_len.
> >
> > Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.")
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
..
>
> Looks ok to me, not tried to compile it though.

Thanks again. I did that and also ran a small recv test that verifies
namelen (but clearly did not help me see the stupid bug I made in
v1..).

Re: [PATCH net v2] packet: in recvmsg msg_name return at least sizeof sockaddr_ll

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 29 Apr 2019 11:46:55 -0400

> From: Willem de Bruijn <willemb@google.com>
> 
> Packet send checks that msg_name is at least sizeof sockaddr_ll.
> Packet recv must return at least this length, so that its output
> can be passed unmodified to packet send.
> 
> This ceased to be true since adding support for lladdr longer than
> sll_addr. Since, the return value uses true address length.
> 
> Always return at least sizeof sockaddr_ll, even if address length
> is shorter. Zero the padding bytes.
> 
> Change v1->v2: do not overwrite zeroed padding again. use copy_len.
> 
> Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.")
> Suggested-by: David Laight <David.Laight@aculab.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied and queued up for -stable.