Skip to contentSkip to conversation

ieee802154: lowpan_header_create check must check daddr

PATCH In review Willem de Bruijn 3 participants 6 messages Dec 23, 2018 – Dec 24, 2018

Subsystems (3)
Networking Network drivers IEEE 802.15.4
lore
Share & tools
lore

Willem de Bruijn

author
#0 lore raw

PATCH 1/1 ieee802154: lowpan_header_create check must check daddr

From: Willem de Bruijn <[email protected]>

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 <[email protected]>

---

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(+)
net/ieee802154/6lowpan/tx.c +3 -0
index ca53efa17be1..8bec827081cd 100644
@@ -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?
 	 */
Signature · 1 line
2.20.1.415.g653613c723-goog
Acked-by on v1 1/1
#1 lore raw

Hi,

thanks Willem to take a look into these callbacks.

Quoted 5 lines · Willem de Bruijn

From: Willem de Bruijn <[email protected]>

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

Ok.

Quoted 3 lines
Fixes 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
Signed-off-by Willem de Bruijn <[email protected]>
Acked-by Alexander Aring <[email protected]>
Quoted 5 lines

---

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.

Quoted 6 lines

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

#2 lore raw

Hi,

...

Quoted 8 lines

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

author
#3 lore raw
In reply to Alexander Aring
Quoted 12 lines · Alexander Aring

Hi,

thanks Willem to take a look into these callbacks.

Quoted 5 lines · Willem de Bruijn

From: Willem de Bruijn <[email protected]>

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

Quoted 7 lines

Ok.

Quoted 3 lines
Fixes 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
Signed-off-by Willem de Bruijn <[email protected]>
Acked-by Alexander Aring <[email protected]>

Thanks Alexander.

Quoted 9 lines
Quoted 6 lines

---

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.

Quoted 6 lines

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.

Quoted 12 lines
Quoted 6 lines

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.

Quoted 7 lines

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.

#4 lore raw

On Sun, Dec 23, 2018 at 07:45:08PM -0500, Willem de Bruijn wrote:
...

Quoted 5 lines
Quoted 2 lines

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.

Quoted 20 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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).

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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

...

Quoted 9 lines
Quoted 1 line

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.

Quoted 5 lines

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