From: Willem de Bruijn
> Sent: 24 April 2019 20:35
> On Wed, Apr 24, 2019 at 3:14 PM Willem de Bruijn
> <willemdebruijn.ker...@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
> > <willemdebruijn.ker...@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> > > <willemdebruijn.ker...@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > > > <willemdebruijn.ker...@gmail.com> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2019 at 11:52 AM David Laight
> > > > > <david.lai...@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.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.
> > > > > >
> > > > > > 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)
