| Message ID | 20190426192735.145633-1-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] packet: validate msg_namelen in send directly | expand |
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)
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);
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)
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.
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;