[PATCH net] ieee802154: lowpan_header_create check must check daddr

[PATCH net] ieee802154: lowpan_header_create check must check daddr

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Packet sockets may call dev_header_parse with NULL daddr. Make
lowpan_header_ops.create fail.

Fixes: 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Re: function comment on packet socket address length: that is (now)
verified to be at least dev->addr_len.

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
the same by callers, so returning -EINVAL.

Is the return 0 on !ETH_P_IPV6 intentional, or should that also be
negative?
---
 net/ieee802154/6lowpan/tx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index ca53efa17be1..8bec827081cd 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -48,6 +48,9 @@ int lowpan_header_create(struct sk_buff *skb, struct net_device *ldev,
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct neighbour *n;
 
+	if (!daddr)
+		return -EINVAL;
+
 	/* TODO:
 	 * if this package isn't ipv6 one, where should it be routed?
 	 */
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

[Alexander Aring]

Hi,

thanks Willem to take a look into these callbacks.

On Sun, Dec 23, 2018 at 12:52:18PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Packet sockets may call dev_header_parse with NULL daddr. Make
> lowpan_header_ops.create fail.
> 

Ok.

> Fixes: 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 

Acked-by: Alexander Aring <aring@mojatatu.com>

> ---
> 
> Re: function comment on packet socket address length: that is (now)
> verified to be at least dev->addr_len.
> 

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.

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.

> 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
> the same by callers, so returning -EINVAL.
> 
> Is the return 0 on !ETH_P_IPV6 intentional, or should that also be
> negative?

Should be, maybe not supported. The function of a lowpan device here is
just header "transforming". I used "transforming" here because it's still
an IPv6 header afterwards (or more) current case is more compression
only.

I need to admit, I never tried AF_PACKET on a lowpan interface but I
thought about it that it ends in bad things... I would like to forbid
it, because they should use RAW IPv6 sockets where at least we already
have code to check that we have at least a IPv6 header at
skb_packer_header() (I hope this is how it works).

Is there any way to do that?

- Alex

Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

[Alexander Aring]

Hi,

...

> I need to admit, I never tried AF_PACKET on a lowpan interface but I
> thought about it that it ends in bad things... I would like to forbid
> it, because they should use RAW IPv6 sockets where at least we already
> have code to check that we have at least a IPv6 header at
> skb_packer_header() (I hope this is how it works).
> 
> Is there any way to do that?
> 

At least forbid the sending via AF_PACKET, receiving has of course a
use case for all protocol analyzers outside.

- Alex

Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

[Willem de Bruijn]

On Sun, Dec 23, 2018 at 3:45 PM Alexander Aring <aring@mojatatu.com> wrote:
>
> Hi,
>
> thanks Willem to take a look into these callbacks.
>
> On Sun, Dec 23, 2018 at 12:52:18PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Packet sockets may call dev_header_parse with NULL daddr. Make
> > lowpan_header_ops.create fail.
> >
>
> Ok.
>
> > Fixes: 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
>
> Acked-by: Alexander Aring <aring@mojatatu.com>

Thanks Alexander.

>
> > ---
> >
> > Re: function comment on packet socket address length: that is (now)
> > verified to be at least dev->addr_len.
> >
>
> 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?

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.

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.

> > 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
> > the same by callers, so returning -EINVAL.
> >
> > Is the return 0 on !ETH_P_IPV6 intentional, or should that also be
> > negative?
>
> Should be, maybe not supported. The function of a lowpan device here is
> just header "transforming". I used "transforming" here because it's still
> an IPv6 header afterwards (or more) current case is more compression
> only.
>
> I need to admit, I never tried AF_PACKET on a lowpan interface but I
> thought about it that it ends in bad things... I would like to forbid
> it, because they should use RAW IPv6 sockets where at least we already
> have code to check that we have at least a IPv6 header at
> skb_packer_header() (I hope this is how it works).
>
> 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.

Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

[Alexander Aring]

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

Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Sun, 23 Dec 2018 12:52:18 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> Packet sockets may call dev_header_parse with NULL daddr. Make
> lowpan_header_ops.create fail.
> 
> Fixes: 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied and queued up for -stable, thanks Willem.