information leak in struct sockaddr_ieee802154 processing

information leak in struct sockaddr_ieee802154 processing

[Lennert Buytenhek]

Hi!

There is a kernel stack information leak in net/ieee802154/socket.c,
in the dgram_recvmsg() handling of socket addresses.

The AF_IEEE802154 sockaddr looks as follows:

	struct ieee802154_addr_sa {
		int addr_type;
		u16 pan_id;
		union {
			u8 hwaddr[IEEE802154_ADDR_LEN];
			u16 short_addr;
		};
	};

	struct sockaddr_ieee802154 {
		sa_family_t family; /* AF_IEEE802154 */
		struct ieee802154_addr_sa addr;
	};

On most architectures that Linux runs on, there will be implicit
structure padding here, in two different places:

* In struct sockaddr_ieee802154, two bytes of padding between 'family'
  (unsigned short) and 'addr', so that 'addr' starts on a four byte
  boundary.

* In struct ieee802154_addr_sa, two bytes at the end of the structure,
  to make the structure 16 bytes.

When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
ieee802154 stack constructs a sockaddr_ieee802154 on the kernel stack
(stack space allocated in net/socket.c, SYSCALL_DEFINE6(recvfrom, [..])
and/or __sys_recvmsg()) without clearing these padding fields, and
between four and ten bytes (depending on the addr_type) of uncleared
kernel stack will be copied to userspace.

A suggested fix is below, however, this unconditionally inserts
explicit padding even on architectures that didn't insert implicit
padding in the first place.

I tested with cross-compilers for aarch64, alpha, arm, avr32, bfin,
c6x, cris, frv, h8300, hppa, hppa64, ia64, m32r, m68k, microblaze,
mips64, mn10300, nios2, powerpc64, ppc64, s390x, sh, sh64, sparc64,
tile, x86_64 and xtensa (every architecture that my Fedora box has
cross-compilers for), and out of these, avr32/cris/h8300/m68k don't
insert any implicit padding, while all the other architectures do
insert 2 bytes of padding in each of the two places described above.

As far as I can tell, the Linux kernel doesn't run on h8300, so
that means that we either unconditionally insert the padding as per
the patch below and break the userland ABI on avr32/cris/m68k in the
process, which doesn't seem like a very attractive option, or
conditionalise the padding on the architecture we're running on
which will get rather messy.

Any thoughts?


thanks,
Lennert



commit b973b3b4098d40f5e9608edd39d80697f246a99f
Author: Lennert Buytenhek <buytenh@wantstofly.org>
Date:   Sun May 17 09:24:06 2015 +0300

    ieee802154: Fix sockaddr_ieee802154 implicit padding information leak.
    
    Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>

diff --git a/include/net/af_ieee802154.h b/include/net/af_ieee802154.h
index 7d38e2f..fe3c221 100644
--- a/include/net/af_ieee802154.h
+++ b/include/net/af_ieee802154.h
@@ -39,6 +39,7 @@ struct ieee802154_addr_sa {
 		u8 hwaddr[IEEE802154_ADDR_LEN];
 		u16 short_addr;
 	};
+	u16 __pad;
 };
 
 #define IEEE802154_PANID_BROADCAST	0xffff
@@ -47,6 +48,7 @@ struct ieee802154_addr_sa {
 
 struct sockaddr_ieee802154 {
 	sa_family_t family; /* AF_IEEE802154 */
+	u16 __pad;
 	struct ieee802154_addr_sa addr;
 };
 
diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index 94a2970..69452fc 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -207,11 +207,14 @@ static inline void ieee802154_addr_to_sa(struct ieee802154_addr_sa *sa,
 	switch (a->mode) {
 	case IEEE802154_ADDR_SHORT:
 		sa->short_addr = le16_to_cpu(a->short_addr);
+		memset(sa->hwaddr + 2, 0, IEEE802154_ADDR_LEN - 2);
 		break;
 	case IEEE802154_ADDR_LONG:
 		ieee802154_devaddr_to_raw(sa->hwaddr, a->extended_addr);
 		break;
 	}
+
+	sa->__pad = 0;
 }
 
 /*
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index b60c65f..22f85bc 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -740,6 +740,7 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 
 	if (saddr) {
 		saddr->family = AF_IEEE802154;
+		saddr->__pad = 0;
 		ieee802154_addr_to_sa(&saddr->addr, &mac_cb(skb)->source);
 		*addr_len = sizeof(*saddr);
 	}

Re: information leak in struct sockaddr_ieee802154 processing

[Lennert Buytenhek]

On Mon, May 25, 2015 at 04:17:12PM +0300, Lennert Buytenhek wrote:

> There is a kernel stack information leak in net/ieee802154/socket.c,
> in the dgram_recvmsg() handling of socket addresses.
> 
> The AF_IEEE802154 sockaddr looks as follows:
> 
> 	struct ieee802154_addr_sa {
> 		int addr_type;
> 		u16 pan_id;
> 		union {
> 			u8 hwaddr[IEEE802154_ADDR_LEN];
> 			u16 short_addr;
> 		};
> 	};
> 
> 	struct sockaddr_ieee802154 {
> 		sa_family_t family; /* AF_IEEE802154 */
> 		struct ieee802154_addr_sa addr;
> 	};
> 
> On most architectures that Linux runs on, there will be implicit
> structure padding here, in two different places:
> 
> * In struct sockaddr_ieee802154, two bytes of padding between 'family'
>   (unsigned short) and 'addr', so that 'addr' starts on a four byte
>   boundary.
> 
> * In struct ieee802154_addr_sa, two bytes at the end of the structure,
>   to make the structure 16 bytes.
> 
> When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
> ieee802154 stack constructs a sockaddr_ieee802154 on the kernel stack
> (stack space allocated in net/socket.c, SYSCALL_DEFINE6(recvfrom, [..])
> and/or __sys_recvmsg()) without clearing these padding fields, and
> between four and ten bytes (depending on the addr_type) of uncleared
> kernel stack will be copied to userspace.
> 
> A suggested fix is below, however, this unconditionally inserts
> explicit padding even on architectures that didn't insert implicit
> padding in the first place.
> 
> I tested with cross-compilers for aarch64, alpha, arm, avr32, bfin,
> c6x, cris, frv, h8300, hppa, hppa64, ia64, m32r, m68k, microblaze,
> mips64, mn10300, nios2, powerpc64, ppc64, s390x, sh, sh64, sparc64,
> tile, x86_64 and xtensa (every architecture that my Fedora box has
> cross-compilers for), and out of these, avr32/cris/h8300/m68k don't
> insert any implicit padding, while all the other architectures do
> insert 2 bytes of padding in each of the two places described above.
> 
> As far as I can tell, the Linux kernel doesn't run on h8300, so
> that means that we either unconditionally insert the padding as per
> the patch below and break the userland ABI on avr32/cris/m68k in the
> process, which doesn't seem like a very attractive option, or
> conditionalise the padding on the architecture we're running on
> which will get rather messy.
> 
> Any thoughts?

How about this way of solving it instead -- any thoughts on this?


 From 8e8aef6d7badb33f50ca04c03a7c884b95733f16 Mon Sep 17 00:00:00 2001
From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Sun, 17 May 2015 09:24:06 +0300
Subject: [PATCH] ieee802154: Fix sockaddr_ieee802154 implicit padding
 information leak.

The AF_IEEE802154 sockaddr looks like this:

	struct sockaddr_ieee802154 {
		sa_family_t family; /* AF_IEEE802154 */
		struct ieee802154_addr_sa addr;
	};

	struct ieee802154_addr_sa {
		int addr_type;
		u16 pan_id;
		union {
			u8 hwaddr[IEEE802154_ADDR_LEN];
			u16 short_addr;
		};
	};

On most architectures there will be implicit structure padding here,
in two different places:

* In struct sockaddr_ieee802154, two bytes of padding between 'family'
  (unsigned short) and 'addr', so that 'addr' starts on a four byte
  boundary.

* In struct ieee802154_addr_sa, two bytes at the end of the structure,
  to make the structure 16 bytes.

When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
ieee802154 stack constructs a struct sockaddr_ieee802154 on the
kernel stack without clearing these padding fields, and, depending
on the addr_type, between four and ten bytes of uncleared kernel
stack will be copied to userspace.

We can't just insert two 'u16 __pad's in the right places and zero
those before copying an address to userspace, as not all architectures
insert this implicit padding -- from a quick test it seems that avr32,
cris and m68k don't insert this padding, while every other architecture
that I have cross compilers for does insert this padding.

The easiest way to plug the leak is to just memset the whole struct
sockaddr_ieee802154 before filling in the fields we want to fill in,
and that's what this patch does.

Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
---
 net/ieee802154/socket.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 02abef2..b6eacf3 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -731,6 +731,12 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	sock_recv_ts_and_drops(msg, sk, skb);
 
 	if (saddr) {
+		/* Clear the implicit padding in struct sockaddr_ieee802154
+		 * (16 bits between 'family' and 'addr') and in struct
+		 * ieee802154_addr_sa (16 bits at the end of the structure).
+		 */
+		memset(saddr, 0, sizeof(*saddr));
+
 		saddr->family = AF_IEEE802154;
 		ieee802154_addr_to_sa(&saddr->addr, &mac_cb(skb)->source);
 		*addr_len = sizeof(*saddr);
-- 
2.4.1

Re: information leak in struct sockaddr_ieee802154 processing

[Alexander Aring]

Hi Lennert,

On Mon, Jun 01, 2015 at 04:45:51PM +0300, Lennert Buytenhek wrote:
...
> When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
> ieee802154 stack constructs a struct sockaddr_ieee802154 on the
> kernel stack without clearing these padding fields, and, depending
> on the addr_type, between four and ten bytes of uncleared kernel
> stack will be copied to userspace.
> 
> We can't just insert two 'u16 __pad's in the right places and zero
> those before copying an address to userspace, as not all architectures
> insert this implicit padding -- from a quick test it seems that avr32,
> cris and m68k don't insert this padding, while every other architecture
> that I have cross compilers for does insert this padding.
> 
> The easiest way to plug the leak is to just memset the whole struct
> sockaddr_ieee802154 before filling in the fields we want to fill in,
> and that's what this patch does.
> 
> Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
> ---
>  net/ieee802154/socket.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> index 02abef2..b6eacf3 100644
> --- a/net/ieee802154/socket.c
> +++ b/net/ieee802154/socket.c
> @@ -731,6 +731,12 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  	sock_recv_ts_and_drops(msg, sk, skb);
>  
>  	if (saddr) {
> +		/* Clear the implicit padding in struct sockaddr_ieee802154
> +		 * (16 bits between 'family' and 'addr') and in struct
> +		 * ieee802154_addr_sa (16 bits at the end of the structure).
> +		 */
> +		memset(saddr, 0, sizeof(*saddr));
> +
>  		saddr->family = AF_IEEE802154;
>  		ieee802154_addr_to_sa(&saddr->addr, &mac_cb(skb)->source);
>  		*addr_len = sizeof(*saddr);


go ahead and send patches, I never looked much into the socket code and
there are many things broken which ends in some crazy behaviour. I don't
know if I can say that, when I am the maintainer... but this code need
some rework/cleanup and such things. Marcel already told that we should
look how bluetooth deals with socket code. I am currently search
somebody which wants to cleanup this socket code so proprietary which
fits on top of 802.15.4 can use this in userspace.

I think your note is exact what David Laight notes some years ago on
netdev, but nobody really cared about that issue. See [0]. This was only
an internal handled struct, but David Laight was on the right track by
note this with structs which are used for kernel<->userspace
communication. (What I mean he note the padding stuff but on the wrong
struct. But the correct struct has this issue also.).

Anyway, big THANK YOU for detecting this and making fix. Feel free to
send a patch, but then it's the question "should we fix this patch into
stable", is it really a big security issue?

btw: The address field should be noted as __le16 also for userspace
communication. _When_ the address fields are not really little endian.

- Alex

[0] http://www.spinics.net/lists/netdev/msg275233.html

Re: information leak in struct sockaddr_ieee802154 processing

[Lennert Buytenhek]

On Mon, Jun 01, 2015 at 04:00:41PM +0200, Alexander Aring wrote:

> Hi Lennert,

Hi!


> > When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
> > ieee802154 stack constructs a struct sockaddr_ieee802154 on the
> > kernel stack without clearing these padding fields, and, depending
> > on the addr_type, between four and ten bytes of uncleared kernel
> > stack will be copied to userspace.
> > 
> > We can't just insert two 'u16 __pad's in the right places and zero
> > those before copying an address to userspace, as not all architectures
> > insert this implicit padding -- from a quick test it seems that avr32,
> > cris and m68k don't insert this padding, while every other architecture
> > that I have cross compilers for does insert this padding.
> > 
> > The easiest way to plug the leak is to just memset the whole struct
> > sockaddr_ieee802154 before filling in the fields we want to fill in,
> > and that's what this patch does.
> > 
> > Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
> > ---
> >  net/ieee802154/socket.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> > index 02abef2..b6eacf3 100644
> > --- a/net/ieee802154/socket.c
> > +++ b/net/ieee802154/socket.c
> > @@ -731,6 +731,12 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  	sock_recv_ts_and_drops(msg, sk, skb);
> >  
> >  	if (saddr) {
> > +		/* Clear the implicit padding in struct sockaddr_ieee802154
> > +		 * (16 bits between 'family' and 'addr') and in struct
> > +		 * ieee802154_addr_sa (16 bits at the end of the structure).
> > +		 */
> > +		memset(saddr, 0, sizeof(*saddr));
> > +
> >  		saddr->family = AF_IEEE802154;
> >  		ieee802154_addr_to_sa(&saddr->addr, &mac_cb(skb)->source);
> >  		*addr_len = sizeof(*saddr);
> 
> go ahead and send patches,

OK, can I submit this with your ACK?


> I never looked much into the socket code and
> there are many things broken which ends in some crazy behaviour. I don't
> know if I can say that, when I am the maintainer... but this code need
> some rework/cleanup and such things. Marcel already told that we should
> look how bluetooth deals with socket code. I am currently search
> somebody which wants to cleanup this socket code so proprietary which
> fits on top of 802.15.4 can use this in userspace.

I'm looking at cleaning this up -- I went through the zigbee specs
to figure out what I think a userspace interface needs to provide,
but, I don't have a zigbee stack to test with or against.


> I think your note is exact what David Laight notes some years ago on
> netdev, but nobody really cared about that issue. See [0]. This was only
> an internal handled struct, but David Laight was on the right track by
> note this with structs which are used for kernel<->userspace
> communication. (What I mean he note the padding stuff but on the wrong
> struct. But the correct struct has this issue also.).

ACK.


> Anyway, big THANK YOU for detecting this and making fix. Feel free to
> send a patch, but then it's the question "should we fix this patch into
> stable", is it really a big security issue?

:-)  I think it's probably worth cc'ing stable@, as the fix
addresses a real issue, and is minimally intrusive and is unlikely
to break anything.


> btw: The address field should be noted as __le16 also for userspace
> communication. _When_ the address fields are not really little endian.

As far as I understand, in sockaddr_ieee802154:

* sa_family (sa_family_t) is always in CPU (host) byte order, as
  the sa_family member of struct sockaddr is always treated as being
  in host byte order

* addr.addr_type (int) is treated as being in host byte order in the
  kernel, with direct comparisons against IEEE802154_ADDR_SHORT and
  IEEE802154_ADDR_LONG in various places without any byteswapping.

* addr.pan_id (u16) is treated as being in host byte order, as there
  is a (in my opinion incorrect ;-)) cpu_to_le16() conversion done on
  it when copying the value into the kernel, and a le16_to_cpu() when
  copying it out of the kernel.

* addr.short_addr (u16), same thing here, this looks like a host byte
  order value, as cpu_to_le16() conversion is done when the value
  enters the kernel, and le16_to_cpu() when being copied back to
  userspace.

* addr.hwaddr[IEEE802154_ADDR_LEN] (u8) has the OUI bytes first.

In other words, I don't think that there are any struct
sockaddr_ieee802154 member fields that should have the __le16 type
-- or did I miss something?

Re: information leak in struct sockaddr_ieee802154 processing

[Alexander Aring]

On Wed, Jun 03, 2015 at 09:45:47AM +0300, Lennert Buytenhek wrote:
> On Mon, Jun 01, 2015 at 04:00:41PM +0200, Alexander Aring wrote:
> 
> > Hi Lennert,
> 
> Hi!
> 
> 
> > > When calling recvmsg(2) on a PF_IEEE802154 SOCK_DGRAM socket, the
> > > ieee802154 stack constructs a struct sockaddr_ieee802154 on the
> > > kernel stack without clearing these padding fields, and, depending
> > > on the addr_type, between four and ten bytes of uncleared kernel
> > > stack will be copied to userspace.
> > > 
> > > We can't just insert two 'u16 __pad's in the right places and zero
> > > those before copying an address to userspace, as not all architectures
> > > insert this implicit padding -- from a quick test it seems that avr32,
> > > cris and m68k don't insert this padding, while every other architecture
> > > that I have cross compilers for does insert this padding.
> > > 
> > > The easiest way to plug the leak is to just memset the whole struct
> > > sockaddr_ieee802154 before filling in the fields we want to fill in,
> > > and that's what this patch does.
> > > 
> > > Signed-off-by: Lennert Buytenhek <buytenh@wantstofly.org>
> > > ---
> > >  net/ieee802154/socket.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> > > index 02abef2..b6eacf3 100644
> > > --- a/net/ieee802154/socket.c
> > > +++ b/net/ieee802154/socket.c
> > > @@ -731,6 +731,12 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > >  	sock_recv_ts_and_drops(msg, sk, skb);
> > >  
> > >  	if (saddr) {
> > > +		/* Clear the implicit padding in struct sockaddr_ieee802154
> > > +		 * (16 bits between 'family' and 'addr') and in struct
> > > +		 * ieee802154_addr_sa (16 bits at the end of the structure).
> > > +		 */
> > > +		memset(saddr, 0, sizeof(*saddr));
> > > +
> > >  		saddr->family = AF_IEEE802154;
> > >  		ieee802154_addr_to_sa(&saddr->addr, &mac_cb(skb)->source);
> > >  		*addr_len = sizeof(*saddr);
> > 
> > go ahead and send patches,
> 
> OK, can I submit this with your ACK?
> 

yes.

> 
> > I never looked much into the socket code and
> > there are many things broken which ends in some crazy behaviour. I don't
> > know if I can say that, when I am the maintainer... but this code need
> > some rework/cleanup and such things. Marcel already told that we should
> > look how bluetooth deals with socket code. I am currently search
> > somebody which wants to cleanup this socket code so proprietary which
> > fits on top of 802.15.4 can use this in userspace.
> 
> I'm looking at cleaning this up -- I went through the zigbee specs
> to figure out what I think a userspace interface needs to provide,
> but, I don't have a zigbee stack to test with or against.
> 

ok. There are also some other ToDo's like frame parsing to support the
full parsing functionality. (This parsing is only for checking if the
frame is correct, again I already worked on the same parsing style like
mac80211 and we should follow this as example).

> 
> > I think your note is exact what David Laight notes some years ago on
> > netdev, but nobody really cared about that issue. See [0]. This was only
> > an internal handled struct, but David Laight was on the right track by
> > note this with structs which are used for kernel<->userspace
> > communication. (What I mean he note the padding stuff but on the wrong
> > struct. But the correct struct has this issue also.).
> 
> ACK.
> 
> 
> > Anyway, big THANK YOU for detecting this and making fix. Feel free to
> > send a patch, but then it's the question "should we fix this patch into
> > stable", is it really a big security issue?
> 
> :-)  I think it's probably worth cc'ing stable@, as the fix
> addresses a real issue, and is minimally intrusive and is unlikely
> to break anything.
> 

ok.

> 
> > btw: The address field should be noted as __le16 also for userspace
> > communication. _When_ the address fields are not really little endian.
> 
> As far as I understand, in sockaddr_ieee802154:
> 
> * sa_family (sa_family_t) is always in CPU (host) byte order, as
>   the sa_family member of struct sockaddr is always treated as being
>   in host byte order
> 
> * addr.addr_type (int) is treated as being in host byte order in the
>   kernel, with direct comparisons against IEEE802154_ADDR_SHORT and
>   IEEE802154_ADDR_LONG in various places without any byteswapping.
> 
> * addr.pan_id (u16) is treated as being in host byte order, as there
>   is a (in my opinion incorrect ;-)) cpu_to_le16() conversion done on
>   it when copying the value into the kernel, and a le16_to_cpu() when
>   copying it out of the kernel.
> 
> * addr.short_addr (u16), same thing here, this looks like a host byte
>   order value, as cpu_to_le16() conversion is done when the value
>   enters the kernel, and le16_to_cpu() when being copied back to
>   userspace.
> 
> * addr.hwaddr[IEEE802154_ADDR_LEN] (u8) has the OUI bytes first.
> 
> In other words, I don't think that there are any struct
> sockaddr_ieee802154 member fields that should have the __le16 type
> -- or did I miss something?

No, then we do this in host byte order. I think, I did this wrong in
nl802154, we do that in little endian there, since the netlink
operations supports nla_get_le64 stuff, etc.

If we do that in host byte order here now, then we should also do the
same byteordering stuff in nl802154 for kernel<->userspace. Otherwise
it's just confusing how do deal with interface sockets and then nl802154
interface. Do you agree here?

Maybe we will grab all nl802154 into some list and doing some cut at
kernel version x.y.

- Alex

Re: information leak in struct sockaddr_ieee802154 processing

[Lennert Buytenhek]

On Mon, Jun 01, 2015 at 04:00:41PM +0200, Alexander Aring wrote:

> [...]
>
> I never looked much into the socket code and
> there are many things broken which ends in some crazy behaviour. I don't
> know if I can say that, when I am the maintainer... but this code need
> some rework/cleanup and such things. Marcel already told that we should
> look how bluetooth deals with socket code. I am currently search
> somebody which wants to cleanup this socket code so proprietary which
> fits on top of 802.15.4 can use this in userspace.

I must admit that I haven't looked at the Bluetooth socket code, but
I've noticed for example that the only capability checks done are done
when setting up crypto state (when setting the WPAN_SECURITY /
WPAN_SECURITY_LEVEL sockopts on a socket), and there are no permission
checks done when opening a SOCK_RAW socket, for example.

Also, SOCK_DGRAM sockets return packets with 802.15.4 headers, just
like SOCK_RAW does, which seems rather wrong.  There's even a comment
about this in the socket code:

	static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
				 int noblock, int flags, int *addr_len)
	{
	[...]
		/* FIXME: skip headers if necessary ?! */
		err = skb_copy_datagram_msg(skb, 0, msg, copied);
		if (err)
			goto done;

Unfortunately, this is now enshrined in the userland API/ABI, and
there's nothing we can do about this now. :(

I also don't really understand why the concept of 802.15.4 PANs was
tied 1:1 to Linux netdevs.

It seems that this was done because 802.11 does something similar,
where each ESSID association (in client mode) or ESSID AP instance
is in itself a Linux netdev, but in that case it makes total sense,
because each such netdev looks and feels like an "Ethernet" (802.3)
interface, and presenting e.g. an ESSID association to the kernel
as its own 802.3 interface massively simplifies integrating this
with the rest of the OS, even if the mapping isn't 1:1 and some
things don't work in this mode (for example, you can't generally
add a wlan interface to a bridge group, while this works fine when
you do it with any wired ethernet interface).

For the Ethernet vlan (802.1q) code, it also makes sense to present
VLAN tagging interfaces (eth0.123) to the kernel as individual 802.3
interfaces, as, again, the rest of the system software gets to
interact with an 802.3 interface this way, which most software that
needs to know about L2 headers already knows how to do.

But for 802.15.4, there is no benefit from doing this that I can
think of.  I understand that there is per-PAN MLME state that needs
to be kept, and that one needs a kernel object to tie this state to,
but I just don't see why this needs to be a netdev -- and it doesn't
seem consistent with how the rest of the socket API works.

And the idea of tying the PAN ID to the interface isn't even applied
consistently -- if I set wpan0 to be in PAN 0x1234, and then send a
packet out via wpan0, I still need to explicitly specify the PAN ID
in the sockaddr (and I can specify a different PAN ID here as far as
I can see), so the possible argument that creating per-PAN netdevs
provides a level of abstraction fails to hold as well.

Actually, if you look at the 802.15.4 6lowpan code, what it does
when transmitting a packet is asking the outgoing interface what its
PAN ID is, and then writing that into the outgoing packet:

	static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
	{
	[...]
		sa.pan_id = ieee802154_mlme_ops(real_dev)->get_pan_id(real_dev);

This just doesn't make sense to me -- it would be like requiring users
of 802.1q VLAN interfaces to query the outgoing VLAN interface's VLAN
tag and to insert that tag into outgoing packets themselves, instead
of having the VLAN interface take care of this by itself.

The other downside of letting 6lowpan attach directly to an 802.15.4
interface is that this provides no protection against other 802.15.4
users (such as userland sockets) grabbing a copy of the 6lowpan
encoded frames.  To be fair, there is no standardised higher level
addressing scheme for 802.15.4, as there is no protocol type field
in the 802.15.4 payload (6lowpan defines an initial payload byte
designating the higher level protocol, with 00xxxxxx reserved for
'not 6lowpan', but this is not a 802.15.4-specified mechanism),
but I'm still not convinced that 802.15.4 6lowpan should be using
dev_add_pack() to attach to its underlying 802.15.4 interface.

Not sure if I've missed something here..

Re: information leak in struct sockaddr_ieee802154 processing

[Alexander Aring]

Hi Lennert,

On Wed, Jun 03, 2015 at 05:01:25PM +0300, Lennert Buytenhek wrote:
> On Mon, Jun 01, 2015 at 04:00:41PM +0200, Alexander Aring wrote:
> 
> > [...]
> >
> > I never looked much into the socket code and
> > there are many things broken which ends in some crazy behaviour. I don't
> > know if I can say that, when I am the maintainer... but this code need
> > some rework/cleanup and such things. Marcel already told that we should
> > look how bluetooth deals with socket code. I am currently search
> > somebody which wants to cleanup this socket code so proprietary which
> > fits on top of 802.15.4 can use this in userspace.
> 
> I must admit that I haven't looked at the Bluetooth socket code, but

me too.

> I've noticed for example that the only capability checks done are done
> when setting up crypto state (when setting the WPAN_SECURITY /
> WPAN_SECURITY_LEVEL sockopts on a socket), and there are no permission
> checks done when opening a SOCK_RAW socket, for example.
> 
> Also, SOCK_DGRAM sockets return packets with 802.15.4 headers, just
> like SOCK_RAW does, which seems rather wrong.  There's even a comment
> about this in the socket code:
> 
> 	static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> 				 int noblock, int flags, int *addr_len)
> 	{
> 	[...]
> 		/* FIXME: skip headers if necessary ?! */
> 		err = skb_copy_datagram_msg(skb, 0, msg, copied);
> 		if (err)
> 			goto done;
> 
> Unfortunately, this is now enshrined in the userland API/ABI, and
> there's nothing we can do about this now. :(
> 

I know, I put it on my todo list, but hasn't time to fix that. See [0].

"I also noticed that the raw socket don't put the mac header into
 payload..."

> I also don't really understand why the concept of 802.15.4 PANs was
> tied 1:1 to Linux netdevs.
> 
> It seems that this was done because 802.11 does something similar,
> where each ESSID association (in client mode) or ESSID AP instance
> is in itself a Linux netdev, but in that case it makes total sense,
> because each such netdev looks and feels like an "Ethernet" (802.3)
> interface, and presenting e.g. an ESSID association to the kernel
> as its own 802.3 interface massively simplifies integrating this
> with the rest of the OS, even if the mapping isn't 1:1 and some
> things don't work in this mode (for example, you can't generally
> add a wlan interface to a bridge group, while this works fine when
> you do it with any wired ethernet interface).
> 
> For the Ethernet vlan (802.1q) code, it also makes sense to present
> VLAN tagging interfaces (eth0.123) to the kernel as individual 802.3
> interfaces, as, again, the rest of the system software gets to
> interact with an 802.3 interface this way, which most software that
> needs to know about L2 headers already knows how to do.
> 
> But for 802.15.4, there is no benefit from doing this that I can
> think of.  I understand that there is per-PAN MLME state that needs
> to be kept, and that one needs a kernel object to tie this state to,
> but I just don't see why this needs to be a netdev -- and it doesn't
> seem consistent with how the rest of the socket API works.
> 
> And the idea of tying the PAN ID to the interface isn't even applied
> consistently -- if I set wpan0 to be in PAN 0x1234, and then send a
> packet out via wpan0, I still need to explicitly specify the PAN ID
> in the sockaddr (and I can specify a different PAN ID here as far as
> I can see), so the possible argument that creating per-PAN netdevs
> provides a level of abstraction fails to hold as well.

yes, all known that's why there exists an opentask at [1]:

"cleanup/fix 802.15.4 af raw/dgram socket code. We should use bluetooth 
socket code as example." And to not say the source addresses is part of
them.

Send patches.

> 
> Actually, if you look at the 802.15.4 6lowpan code, what it does
> when transmitting a packet is asking the outgoing interface what its
> PAN ID is, and then writing that into the outgoing packet:
> 
> 	static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
> 	{
> 	[...]
> 		sa.pan_id = ieee802154_mlme_ops(real_dev)->get_pan_id(real_dev);
> 
> This just doesn't make sense to me -- it would be like requiring users
> of 802.1q VLAN interfaces to query the outgoing VLAN interface's VLAN
> tag and to insert that tag into outgoing packets themselves, instead
> of having the VLAN interface take care of this by itself.

6LoWPAN connection is "intra-pan" communication only. Which means source
and destionation pan_id are the same.

> 
> The other downside of letting 6lowpan attach directly to an 802.15.4
> interface is that this provides no protection against other 802.15.4
> users (such as userland sockets) grabbing a copy of the 6lowpan
> encoded frames.  To be fair, there is no standardised higher level
> addressing scheme for 802.15.4, as there is no protocol type field
> in the 802.15.4 payload (6lowpan defines an initial payload byte
> designating the higher level protocol, with 00xxxxxx reserved for
> 'not 6lowpan', but this is not a 802.15.4-specified mechanism),

Yes, the current way is to check on this dispatch value which seems that
it is a 6LoWPAN packet. If this is set correctly we try to parse it as
6LoWPAN, if not we drop it currently. Is there something wrong with this
behaviour? I know we need a better handling for the 6LoWPAN parsing
stuff because we assume there that the skb has the right length and such
stuff, but this is part of the whole reimplementation of frame
parsing/generation rework. (then 802.15.4 6lowpan parsing looks like
mac802154 which based on mac80211 and has hopefully more checks agains
if the headers are right).

I don't know what you want to try to saying with the complete ethernet
vlan stuff. You want to try to find some mapping to doing more pan_id
stuff from userspace level, because you can only control them by the
IPv6 sockets?

- Alex

[0] http://www.spinics.net/lists/linux-wpan/msg01917.html
[1] http://wpan.cakelab.org/
[2] http://lxr.free-electrons.com/source/net/ieee802154/6lowpan/rx.c#L155