[PATCH net] packet: validate address length if non-zero

[PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

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

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 <idosch@idosch.org>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 5dda263b4a0a..eedacdebcd4c 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH net] packet: validate address length if non-zero

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Sat, 22 Dec 2018 16:53:45 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> 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 <idosch@idosch.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied, thanks Willem.

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

Thanks again.

Re: [PATCH net] packet: validate address length if non-zero

[Ido Schimmel]

On Sat, Dec 22, 2018 at 04:53:45PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> 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 <idosch@idosch.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Thanks for the quick fix! Issue is gone with this patch.

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Sat, Dec 22, 2018 at 6:13 PM David Miller <davem@davemloft.net> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Sat, 22 Dec 2018 16:53:45 -0500
>
> > From: Willem de Bruijn <willemb@google.com>
> >
> > 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 <idosch@idosch.org>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> 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.

RE: [PATCH net] packet: validate address length if non-zero

[David Laight]

From: Willem de Bruijn
> 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 <idosch@idosch.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/packet/af_packet.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 5dda263b4a0a..eedacdebcd4c 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -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;
> --
> 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)

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > 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 <idosch@idosch.org>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> >  net/packet/af_packet.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 5dda263b4a0a..eedacdebcd4c 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -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;
> > --
> > 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.

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
>= sizeof(sll_addr), excluding Ethernet.

RE: [PATCH net] packet: validate address length if non-zero

[David Laight]

From: Willem de Bruijn
> Sent: 23 April 2019 16:08
> On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Willem de Bruijn
> > > 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 <idosch@idosch.org>
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > ---
> > >  net/packet/af_packet.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -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;
> > > --
> > > 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.
> 
> 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
> >= 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)

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 23 April 2019 16:08
> > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Willem de Bruijn
> > > > 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 <idosch@idosch.org>
> > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > ---
> > > >  net/packet/af_packet.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > --- a/net/packet/af_packet.c
> > > > +++ b/net/packet/af_packet.c
> > > > @@ -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;
> > > > --
> > > > 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.
> >
> > 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
> > >= 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.

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.

> 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?

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Willem de Bruijn
> > > Sent: 23 April 2019 16:08
> > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Willem de Bruijn
> > > > > 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 <idosch@idosch.org>
> > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > ---
> > > > >  net/packet/af_packet.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > --- a/net/packet/af_packet.c
> > > > > +++ b/net/packet/af_packet.c
> > > > > @@ -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;
> > > > > --
> > > > > 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.
> > >
> > > 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
> > > >= 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.
>
> 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.

>
> > 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?

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Willem de Bruijn
> > > > Sent: 23 April 2019 16:08
> > > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > > >
> > > > > From: Willem de Bruijn
> > > > > > 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 <idosch@idosch.org>
> > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > > ---
> > > > > >  net/packet/af_packet.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > > --- a/net/packet/af_packet.c
> > > > > > +++ b/net/packet/af_packet.c
> > > > > > @@ -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;
> > > > > > --
> > > > > > 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.
> > > >
> > > > 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
> > > > >= 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.
> >
> > 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.

So something like

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 5c4a118d6f969..64ab3c960f538 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Willem de Bruijn
> > > > > Sent: 23 April 2019 16:08
> > > > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > >
> > > > > > From: Willem de Bruijn
> > > > > > > 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 <idosch@idosch.org>
> > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > > > ---
> > > > > > >  net/packet/af_packet.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > > > --- a/net/packet/af_packet.c
> > > > > > > +++ b/net/packet/af_packet.c
> > > > > > > @@ -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;
> > > > > > > --
> > > > > > > 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.
> > > > >
> > > > > 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
> > > > > >= 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.
> > >
> > > 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.
>
> So something like
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 5c4a118d6f969..64ab3c960f538 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -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;

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.

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Wed, Apr 24, 2019 at 3:14 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> > > > >
> > > > > From: Willem de Bruijn
> > > > > > Sent: 23 April 2019 16:08
> > > > > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > > >
> > > > > > > From: Willem de Bruijn
> > > > > > > > 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 <idosch@idosch.org>
> > > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > > > > ---
> > > > > > > >  net/packet/af_packet.c | 4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > > > > --- a/net/packet/af_packet.c
> > > > > > > > +++ b/net/packet/af_packet.c
> > > > > > > > @@ -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;
> > > > > > > > --
> > > > > > > > 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.
> > > > > >
> > > > > > 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
> > > > > > >= 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.
> > > >
> > > > 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.
> >
> > So something like
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 5c4a118d6f969..64ab3c960f538 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -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;
>
> 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.

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

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

This, then, still stands.

RE: [PATCH net] packet: validate address length if non-zero

[David Laight]

From: Willem de Bruijn
> Sent: 24 April 2019 20:35
> On Wed, Apr 24, 2019 at 3:14 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > >
> > > > > > From: Willem de Bruijn
> > > > > > > Sent: 23 April 2019 16:08
> > > > > > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > > > >
> > > > > > > > From: Willem de Bruijn
> > > > > > > > > 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 <idosch@idosch.org>
> > > > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > > > > > ---
> > > > > > > > >  net/packet/af_packet.c | 4 ++--
> > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > > > > > --- a/net/packet/af_packet.c
> > > > > > > > > +++ b/net/packet/af_packet.c
> > > > > > > > > @@ -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;
> > > > > > > > > --
> > > > > > > > > 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.
> > > > > > >
> > > > > > > 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
> > > > > > > >= 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.
> > > > >
> > > > > 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.
> > >
> > > So something like
> > >
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 5c4a118d6f969..64ab3c960f538 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -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;
> >
> > 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.
> 
> 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.

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

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Thu, Apr 25, 2019 at 5:32 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 24 April 2019 20:35
> > On Wed, Apr 24, 2019 at 3:14 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > > >
> > > > > > > From: Willem de Bruijn
> > > > > > > > Sent: 23 April 2019 16:08
> > > > > > > > On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@aculab.com> wrote:
> > > > > > > > >
> > > > > > > > > From: Willem de Bruijn
> > > > > > > > > > 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 <idosch@idosch.org>
> > > > > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > > > > > > ---
> > > > > > > > > >  net/packet/af_packet.c | 4 ++--
> > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > > > > > > > index 5dda263b4a0a..eedacdebcd4c 100644
> > > > > > > > > > --- a/net/packet/af_packet.c
> > > > > > > > > > +++ b/net/packet/af_packet.c
> > > > > > > > > > @@ -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;
> > > > > > > > > > --
> > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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
> > > > > > > > >= 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.
> > > > > >
> > > > > > 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.
> > > >
> > > > So something like
> > > >
> > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > index 5c4a118d6f969..64ab3c960f538 100644
> > > > --- a/net/packet/af_packet.c
> > > > +++ b/net/packet/af_packet.c
> > > > @@ -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;
> > >
> > > 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.
> >
> > 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.
>
> > > 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.

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

> 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.

> 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.



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

RE: [PATCH net] packet: validate address length if non-zero

[David Laight]

From: Willem de Bruijn
> Sent: 25 April 2019 14:57
...
> > 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.
> 
> 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 :-)

> > 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.

Indeed.

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

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Thu, Apr 25, 2019 at 10:35 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 25 April 2019 14:57
> ...
> > > 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.
> >
> > 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 :-)
>
> > > 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.
>
> Indeed.
>
> > 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!

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.

> 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.

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Thu, Apr 25, 2019 at 11:42 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Apr 25, 2019 at 10:35 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Willem de Bruijn
> > > Sent: 25 April 2019 14:57
> > ...
> > > > 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.
> > >
> > > 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 :-)
> >
> > > > 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.

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

RE: [PATCH net] packet: validate address length if non-zero

[David Laight]

From: Willem de Bruijn
> Sent: 26 April 2019 16:11
> On Thu, Apr 25, 2019 at 11:42 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Thu, Apr 25, 2019 at 10:35 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Willem de Bruijn
> > > > Sent: 25 April 2019 14:57
> > > ...
> > > > > 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.
> > > >
> > > > 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 :-)
> > >
> > > > > 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.
> 
> 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)

Re: [PATCH net] packet: validate address length if non-zero

[Willem de Bruijn]

On Fri, Apr 26, 2019 at 11:14 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 26 April 2019 16:11
> > On Thu, Apr 25, 2019 at 11:42 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Thu, Apr 25, 2019 at 10:35 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Willem de Bruijn
> > > > > Sent: 25 April 2019 14:57
> > > > ...
> > > > > > 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.
> > > > >
> > > > > 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 :-)
> > > >
> > > > > > 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.
> >
> > 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.

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/