lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF=yD-+HmVawOQzLn1UZzb2CpE9B8Hvw5_H2y5L4_za8KkU8ng@mail.gmail.com> Date: Tue, 23 Apr 2019 11:07:47 -0400 From: Willem de Bruijn <willemdebruijn.kernel@...il.com> To: David Laight <David.Laight@...lab.com> Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>, "idosch@...sch.org" <idosch@...sch.org>, Willem de Bruijn <willemb@...gle.com> Subject: Re: [PATCH net] packet: validate address length if non-zero On Tue, Apr 23, 2019 at 5:59 AM David Laight <David.Laight@...lab.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@...sch.org> > > Signed-off-by: Willem de Bruijn <willemb@...gle.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.
Powered by blists - more mailing lists