netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] packet: validate msg_namelen in send directly
@ 2019-04-24 19:07 Willem de Bruijn
  0 siblings, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2019-04-24 19:07 UTC (permalink / raw)
  To: netdev; +Cc: davem, David.Laight, idosch, Willem de Bruijn

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

Packet sockets in datagram mode take a link layer destination address
in sockaddr_ll.sll_addr. For some device types, address length exceeds
this field size, extending beyond the struct.

This addr argument is then passed to dev_hard_header without length
and assumed to be equal to dev->addr_len. Before commit 99137b7888f4
("packet: validate address length") it was possible to pass a shorter
msg_name and msg_namelen, causing an out of bound read.

After that and commit 6b8d95f1795c4 ("packet: validate address length
if non-zero") send() returns EINVAL if sockaddr_ll.sll_halen is
non-zero but less than dev->addr_len (zero length is allowed because
sockets in SOCK_RAW mode ignore sll_addr).

It turns out that legacy applications require more permissive input.
An otherwise legitimate Ethernet application that sets sll_halen > 0,
< ETH_ALEN sees EINVAL, even though ETH_ALEN fits in sockaddr_ll, so
cannot cause the out of bounds read.

Be more permissive. Ignore sll_halen, which is not used elsewhere.
Directly compare msg_namelen to dev->addr_len.

Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9419c5cf4de5e..9ae30bbd00913 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2616,6 +2616,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		proto	= po->num;
 		addr	= NULL;
 	} else {
+		const int sa_len = offsetof(struct sockaddr_ll, sll_addr);
+		const short sk_type = po->sk.sk_socket->type;
+
 		err = -EINVAL;
 		if (msg->msg_namelen < sizeof(struct sockaddr_ll))
 			goto out;
@@ -2624,9 +2627,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 						sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		addr	= saddr->sll_halen ? saddr->sll_addr : NULL;
+		addr	= sk_type == SOCK_DGRAM ? 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)
+		if (addr && dev && msg->msg_namelen < (sa_len + dev->addr_len))
 			goto out_put;
 	}
 
@@ -2818,15 +2821,17 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		proto	= po->num;
 		addr	= NULL;
 	} else {
+		const int sa_len = offsetof(struct sockaddr_ll, sll_addr);
+
 		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 < (sa_len + dev->addr_len))
 			goto out_unlock;
 	}
 
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: validate msg_namelen in send directly
  2019-04-29 13:25     ` David Laight
@ 2019-04-29 13:36       ` Willem de Bruijn
  0 siblings, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2019-04-29 13:36 UTC (permalink / raw)
  To: David Laight; +Cc: netdev, davem, idosch, Willem de Bruijn

On Mon, Apr 29, 2019 at 9:25 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 29 April 2019 13:53
> > On Mon, Apr 29, 2019 at 5:00 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Willem de Bruijn
> > > > Sent: 26 April 2019 20:28
> > > > Packet sockets in datagram mode take a destination address. Verify its
> > > > length before passing to dev_hard_header.
> > > >
> > > > Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
> > > > established behavior. Directly compare msg_namelen to dev->addr_len.
> > > >
> > > > Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
> > > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > ---
> > > >  net/packet/af_packet.c | 18 ++++++++++++------
> > > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > index 9419c5cf4de5e..13301e36b4a28 100644
> > > > --- a/net/packet/af_packet.c
> > > > +++ b/net/packet/af_packet.c
> > > > @@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > >                                               sll_addr)))
> > > >                       goto out;
> > > >               proto   = saddr->sll_protocol;
> > > > -             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_put;
> > > > +             if (po->sk.sk_socket->type == SOCK_DGRAM) {
> > > > +                     addr = saddr->sll_addr;
> > > > +                     if (dev && msg->msg_namelen < dev->addr_len +
> > > > +                                     offsetof(struct sockaddr_ll, sll_addr))
> > > > +                             goto out_put;
> > > > +             }
> > >
> > > IIRC you need to initialise 'addr - NULL' at the top of the functions.
> > > I'm surprised the compiler doesn't complain.
> >
> > It did complain when I moved it below the if (dev && ..) branch. But
> > inside a branch with exactly the same condition as the one where used,
> > the compiler did figure it out. Admittedly that is fragile.
>
> Even a function call should be enough since the called code is allowed
> to modify po->sk.sk_socket->type via a global pointer.
>
> > Then it might be simplest to restore the unconditional assignment
> >
> >                 proto   = saddr->sll_protocol;
> > +               addr    = saddr->sll_addr;
> >                 dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
>
> There is an 'addr = NULL' in the 'address absent' branch.
> Moving that higher up makes it even more clear that the address is
> only set in one place.

I can do that. Only, touching code in multiple locations can make
backporting to stable branches more difficult.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH net] packet: validate msg_namelen in send directly
  2019-04-29 12:53   ` Willem de Bruijn
@ 2019-04-29 13:25     ` David Laight
  2019-04-29 13:36       ` Willem de Bruijn
  0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2019-04-29 13:25 UTC (permalink / raw)
  To: 'Willem de Bruijn'; +Cc: netdev, davem, idosch, Willem de Bruijn

From: Willem de Bruijn
> Sent: 29 April 2019 13:53
> On Mon, Apr 29, 2019 at 5:00 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Willem de Bruijn
> > > Sent: 26 April 2019 20:28
> > > Packet sockets in datagram mode take a destination address. Verify its
> > > length before passing to dev_hard_header.
> > >
> > > Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
> > > established behavior. Directly compare msg_namelen to dev->addr_len.
> > >
> > > Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
> > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > ---
> > >  net/packet/af_packet.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 9419c5cf4de5e..13301e36b4a28 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > >                                               sll_addr)))
> > >                       goto out;
> > >               proto   = saddr->sll_protocol;
> > > -             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_put;
> > > +             if (po->sk.sk_socket->type == SOCK_DGRAM) {
> > > +                     addr = saddr->sll_addr;
> > > +                     if (dev && msg->msg_namelen < dev->addr_len +
> > > +                                     offsetof(struct sockaddr_ll, sll_addr))
> > > +                             goto out_put;
> > > +             }
> >
> > IIRC you need to initialise 'addr - NULL' at the top of the functions.
> > I'm surprised the compiler doesn't complain.
> 
> It did complain when I moved it below the if (dev && ..) branch. But
> inside a branch with exactly the same condition as the one where used,
> the compiler did figure it out. Admittedly that is fragile.

Even a function call should be enough since the called code is allowed
to modify po->sk.sk_socket->type via a global pointer.

> Then it might be simplest to restore the unconditional assignment
> 
>                 proto   = saddr->sll_protocol;
> +               addr    = saddr->sll_addr;
>                 dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);

There is an 'addr = NULL' in the 'address absent' branch.
Moving that higher up makes it even more clear that the address is 
only set in one place.

	David

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: validate msg_namelen in send directly
  2019-04-29  9:00 ` David Laight
@ 2019-04-29 12:53   ` Willem de Bruijn
  2019-04-29 13:25     ` David Laight
  0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2019-04-29 12:53 UTC (permalink / raw)
  To: David Laight; +Cc: netdev, davem, idosch, Willem de Bruijn

On Mon, Apr 29, 2019 at 5:00 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 26 April 2019 20:28
> > Packet sockets in datagram mode take a destination address. Verify its
> > length before passing to dev_hard_header.
> >
> > Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
> > established behavior. Directly compare msg_namelen to dev->addr_len.
> >
> > Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> >  net/packet/af_packet.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 9419c5cf4de5e..13301e36b4a28 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >                                               sll_addr)))
> >                       goto out;
> >               proto   = saddr->sll_protocol;
> > -             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_put;
> > +             if (po->sk.sk_socket->type == SOCK_DGRAM) {
> > +                     addr = saddr->sll_addr;
> > +                     if (dev && msg->msg_namelen < dev->addr_len +
> > +                                     offsetof(struct sockaddr_ll, sll_addr))
> > +                             goto out_put;
> > +             }
>
> IIRC you need to initialise 'addr - NULL' at the top of the functions.
> I'm surprised the compiler doesn't complain.

It did complain when I moved it below the if (dev && ..) branch. But
inside a branch with exactly the same condition as the one where used,
the compiler did figure it out. Admittedly that is fragile. Then it
might be simplest to restore the unconditional assignment

                proto   = saddr->sll_protocol;
+               addr    = saddr->sll_addr;
                dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH net] packet: validate msg_namelen in send directly
  2019-04-26 19:27 Willem de Bruijn
@ 2019-04-29  9:00 ` David Laight
  2019-04-29 12:53   ` Willem de Bruijn
  0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2019-04-29  9:00 UTC (permalink / raw)
  To: 'Willem de Bruijn', netdev; +Cc: davem, idosch, Willem de Bruijn

From: Willem de Bruijn
> Sent: 26 April 2019 20:28
> Packet sockets in datagram mode take a destination address. Verify its
> length before passing to dev_hard_header.
> 
> Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
> established behavior. Directly compare msg_namelen to dev->addr_len.
> 
> Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
> Suggested-by: David Laight <David.Laight@aculab.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/packet/af_packet.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 9419c5cf4de5e..13301e36b4a28 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  						sll_addr)))
>  			goto out;
>  		proto	= saddr->sll_protocol;
> -		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_put;
> +		if (po->sk.sk_socket->type == SOCK_DGRAM) {
> +			addr = saddr->sll_addr;
> +			if (dev && msg->msg_namelen < dev->addr_len +
> +					offsetof(struct sockaddr_ll, sll_addr))
> +				goto out_put;
> +		}

IIRC you need to initialise 'addr - NULL' at the top of the functions.
I'm surprised the compiler doesn't complain.

	David

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH net] packet: validate msg_namelen in send directly
@ 2019-04-26 19:27 Willem de Bruijn
  2019-04-29  9:00 ` David Laight
  0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2019-04-26 19:27 UTC (permalink / raw)
  To: netdev; +Cc: davem, David.Laight, idosch, Willem de Bruijn

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

Packet sockets in datagram mode take a destination address. Verify its
length before passing to dev_hard_header.

Prior to 2.6.14-rc3, the send code ignored sll_halen. This is
established behavior. Directly compare msg_namelen to dev->addr_len.

Fixes: 6b8d95f1795c4 ("packet: validate address length if non-zero")
Suggested-by: David Laight <David.Laight@aculab.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9419c5cf4de5e..13301e36b4a28 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2624,10 +2624,13 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 						sll_addr)))
 			goto out;
 		proto	= saddr->sll_protocol;
-		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_put;
+		if (po->sk.sk_socket->type == SOCK_DGRAM) {
+			addr = saddr->sll_addr;
+			if (dev && msg->msg_namelen < dev->addr_len +
+					offsetof(struct sockaddr_ll, sll_addr))
+				goto out_put;
+		}
 	}
 
 	err = -ENXIO;
@@ -2824,10 +2827,13 @@ 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_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_unlock;
+		if (sock->type == SOCK_DGRAM) {
+			addr = saddr->sll_addr;
+			if (dev && msg->msg_namelen < dev->addr_len +
+					offsetof(struct sockaddr_ll, sll_addr))
+				goto out_unlock;
+		}
 	}
 
 	err = -ENXIO;
-- 
2.21.0.593.g511ec345e18-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-29 13:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24 19:07 [PATCH net] packet: validate msg_namelen in send directly Willem de Bruijn
2019-04-26 19:27 Willem de Bruijn
2019-04-29  9:00 ` David Laight
2019-04-29 12:53   ` Willem de Bruijn
2019-04-29 13:25     ` David Laight
2019-04-29 13:36       ` Willem de Bruijn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).