Message-ID: <20181224152129.k2cly453etacodis@x220t>
Date:   Mon, 24 Dec 2018 10:21:29 -0500
From:   Alexander Aring <aring@...atatu.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexander Aring <alex.aring@...il.com>,
        jukka.rissanen@...ux.intel.com,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] ieee802154: lowpan_header_create check must check
 daddr

On Sun, Dec 23, 2018 at 07:45:08PM -0500, Willem de Bruijn wrote:
...
> > I had some questions when I was digging AF_PACKET code. So the UAPI
> > limitation of AF_PACKET has a sockaddr_t of 8 bytes.
> 
> Do you mean the size of sll_addr in struct sockaddr_ll?
> 

yes.

> It is possible to pass larger addresses, such as INFINIBAND_ALEN. See
> also the checks in packet_snd:
> 
>                 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;
> 
> The address passed may exceed sizeof(struct sockaddr_ll), in which
> case the true size of sll_addr is defined by sll_halen. There are
> various hardware lengths well above 8B, such as INFINIBAND_ALEN.
> 
> > What is when I assign e.g. more than 8 bytes to dev->addr_len and
> > copying dev->addr_len to it. Does we care about that? At least some
> > assert warning if somebody try to use larger than 8 bytes dev->addr_len
> > for AF_PACKET dgram sockets which might using these pointers and copy
> > dev->addr_len size? As I already saw it before, but don't know what the
> > best place it is to check on that.
> 
> With the recent addition to verify that sll_halen is greater than or
> equal to dev->addr_len, this should now be handled correctly.
> 

Thanks. I was running into issues because the 802.15.4 address scheme
was using a memcpy() of greater than 8 bytes in header_create()
callback. I did a change to DGRAM AF_PACKET that fits to it as "just
send frames out with unique address", for all other cases the user need
to switch to subsystem specific socket interface.

> On a related note, the commit mentions dev->hard_header_len in
> the context of variable hardware header lengths. This has since been
> clarified. dev->hard_header_len is the maximum header length, a new
> dev->min_header_len is used to validate the minimum size.
> 

Ah, thanks. That is a good idea for a variable length header as in
802.15.4. I will take a look into that.

Before I used hard_header_len as minimum header length (which the driver
can be assume it's there).

> > > It is customary to return -header_len on failure in .create(), but
> > > not sure what that would be here, and any negative value is treated
...
> > Is there any way to do that?
> 
> Practically speaking, skb->sk is set by the time dev_hard_header
> is called, so the packet type could be inferred.
> 
> This is a bigger problem with AF_PACKET (in particular root in userns)
> and more in general validation of packets coming from userspace.
> It is certainly not limited to lowpan.
> 
> User packets need not come only from PF_PACKET. They can also
> come from guest kernels through a tap interface. That is potentially
> more problematic, as from unprivileged sources. For that reason
> blocking AF_PACKET is not a robust solution to these issues.
> Though likely less relevant to lowpan, to be fair.

Okay, thanks for letting me known there is not just AF_PACKET. Currently
we assume there is a IPv6 header in ndo_start_xmit() - I guess we need
more checks that we have it at least there.

Also the use of dev_hard_header() is somehow awkward as it just works
and we didn't run yet in problems like in previous implementation.

As IPv6 use this callback to tell which L2 addresses need to be used by
a ndisc lookup. We putting these information in skb_headroom() and accessing
them in ndo_start_xmit() again, this assumes nobody do a manipulation of
the skb_headroom() in between them.

Note: Not real true for 802.15.4 because we do a ndisc lookup again if
it's not a "internal used only" broadcast address. As said the 802.15.4
address scheme is quite different than what we can currently support in
a netdevice and we currently map a invalid address scope to another one
_internally_.

I didn't find yet another solution to make this better without adding
new "layer interacting" callbacks. :(

- Alex

Powered by blists - more mailing lists