netdev - Re: [PATCH net] packet: validate msg

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Apr 2019 08:53:06 -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 msg_namelen in send directly

On Mon, Apr 29, 2019 at 5:00 AM David Laight <David.Laight@...lab.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@...lab.com>
> > Signed-off-by: Willem de Bruijn <willemb@...gle.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);