On Tue, Apr 23, 2019 at 5:59 AM David Laight <david.lai...@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 <ido...@idosch.org>
> > Signed-off-by: Willem de Bruijn <will...@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.
