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@xxxxxxxxxxxxxx>
> > ---
> > 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
