Message-ID: <CAF=yD-LqmAnJTWUnt3-dPS6w3CsWP7jf+phTKQ2JhkU2CsweqA@mail.gmail.com>
Date:   Thu, 25 Apr 2019 09:56:48 -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 Thu, Apr 25, 2019 at 5:32 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: 24 April 2019 20:35
> > On Wed, Apr 24, 2019 at 3:14 PM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 2:21 PM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 1:21 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2019 at 1:04 PM Willem de Bruijn
> > > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > > >
> > > > > > On Tue, Apr 23, 2019 at 11:52 AM David Laight <David.Laight@...lab.com> wrote:
> > > > > > >
> > > > > > > From: Willem de Bruijn
> > > > > > > > Sent: 23 April 2019 16:08
> > > > > > > > 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.
> > > > > > >
> > > > > > > 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.

Great find, thanks. I hadn't thought of going back that far, but
clearly should in these legacy caller questions..

> 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;
>                         }

Yes, given the above, this looks great to me.

In general I'm hesitant to loosen interface constraints. As you can
never tighten them again. But given the change on recv, it seems
correct here. That is technically a separate issue, so worth a
separate patch, I think. If that's not too pedantic. Else at least an
extra Fixes tag.

> Although it might even be worth moving the check for (dev == NULL)
> inside the conditional.
>
> Then we'll paint the bikeshed in Paisley.

:)

Agreed on moving the dev. That makes it really obvious that addr is
only used in datagram mode. And indeed exactly matches the branch
around the only user, dev_hard_header.



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

Powered by blists - more mailing lists