netdevice notifier and device private data

netdevice notifier and device private data

[Alexander Aring]

Hey netdev community,

I am trying to solve some issue which Eric Dumazet points to me by
commit ca0edb131bdf ("ieee802154: 6lowpan: fix possible NULL deref in
lowpan_device_event()").

The issue is that dev->type can be changed during runtime. We don't have
any problems with the netdevice notifier which Eric Dumazet fixed. I am
bother with another netdevice notifier which is broken because the same
tun/tap feature and I don't have any dev->$SUBSYSTEM_DEV_POINTER to check
if this is my netdevice type.

This netdevice notifier will access the dev->priv area which is only
available for the dev->type which was allocated and initialized with the
right dev->priv room. If a tap/tun netdevice changed their dev->type I
might have an illegal read of netdev->priv and I can't confirm that it
has the data which I cast to it. The reason for that is that tap/tun
netdevices doesn't run my netdevice init.

I already see code outside who changed tun netdevice to the
ARPHRD_6LOWPAN type and I suppose they running into this issue.
(Btw: I don't know why somebody wants to changed that type to
ARPHRD_6LOWPAN on tun).

My question is:

How we deal with that? Is it forbidden to access dev->priv from a
global netdevice notifier which only checks for dev->type?

I could solve it like Eric Dumazet and introduce a special
dev->$SUBSYSTEM_DEV_POINTER and check on it if set. At least tun/tap
will not set these pointers, then I am sure the netdevice was running
through my init function. Seems for me the best solution right now and
I think I will go for it.

I assumed before the data of dev->priv is binded to dev->type.
This tun/tap feature will break at least my handling and I am not sure
if there are others users which using dev->priv in netdevice notifier
and don't check on dev->$SUBSYSTEM_DEV_POINTER if they have one.

Thanks for everybody in advance to solve this issue.

- Alex

Re: netdevice notifier and device private data

[Stephen Hemminger]

On Fri, 8 Jun 2018 13:34:55 -0400
Alexander Aring <aring@mojatatu.com> wrote:

> Hey netdev community,
> 
> I am trying to solve some issue which Eric Dumazet points to me by
> commit ca0edb131bdf ("ieee802154: 6lowpan: fix possible NULL deref in
> lowpan_device_event()").
> 
> The issue is that dev->type can be changed during runtime. We don't have
> any problems with the netdevice notifier which Eric Dumazet fixed. I am
> bother with another netdevice notifier which is broken because the same
> tun/tap feature and I don't have any dev->$SUBSYSTEM_DEV_POINTER to check
> if this is my netdevice type.
> 
> This netdevice notifier will access the dev->priv area which is only
> available for the dev->type which was allocated and initialized with the
> right dev->priv room. If a tap/tun netdevice changed their dev->type I
> might have an illegal read of netdev->priv and I can't confirm that it
> has the data which I cast to it. The reason for that is that tap/tun
> netdevices doesn't run my netdevice init.
> 
> I already see code outside who changed tun netdevice to the
> ARPHRD_6LOWPAN type and I suppose they running into this issue.
> (Btw: I don't know why somebody wants to changed that type to
> ARPHRD_6LOWPAN on tun).
> 
> My question is:
> 
> How we deal with that? Is it forbidden to access dev->priv from a
> global netdevice notifier which only checks for dev->type?
> 
> I could solve it like Eric Dumazet and introduce a special
> dev->$SUBSYSTEM_DEV_POINTER and check on it if set. At least tun/tap
> will not set these pointers, then I am sure the netdevice was running
> through my init function. Seems for me the best solution right now and
> I think I will go for it.
> 
> I assumed before the data of dev->priv is binded to dev->type.
> This tun/tap feature will break at least my handling and I am not sure
> if there are others users which using dev->priv in netdevice notifier
> and don't check on dev->$SUBSYSTEM_DEV_POINTER if they have one.
> 
> Thanks for everybody in advance to solve this issue.
> 
> - Alex

notifiers are always called with RTNL mutex held
and dev->type should not change unless RTNL is held.

Re: netdevice notifier and device private data

[Alexander Aring]

Hi Stephen,

On Fri, Jun 08, 2018 at 11:14:57AM -0700, Stephen Hemminger wrote:
...
> 
> notifiers are always called with RTNL mutex held
> and dev->type should not change unless RTNL is held.

thanks for you answer. I am not talking about any race between notifiers
vs dev->type change.

I am talking that dev->type was already changed and a upcoming notifier ends
in undefined behaviour when it derefences dev->priv. I have some notifier
which maps a cast from dev->type to a specific structure at dev->priv. This
structure is not there in tap/tun devices if they changed to "my" dev->type
and the notifier occurs.

- Alex

Re: netdevice notifier and device private data

[Michael Richardson]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]


Alexander Aring <aring@mojatatu.com> wrote:
    Alex> I already see code outside who changed tun netdevice to the
    Alex> ARPHRD_6LOWPAN type and I suppose they running into this
    Alex> issue.  (Btw: I don't know why somebody wants to changed that
    Alex> type to ARPHRD_6LOWPAN on tun).

so that they can have the kernel do 6lowpan processing, emitting 6lowPAN
packets into userspace to be transfered into a radio via some proprietary
interface (including, for instance SLIP over USB cable to Contiki or OpenWSN stack, 
set up to act as radio only)

-- 
]               Never tell me the odds!                 | ipv6 mesh networks [ 
]   Michael Richardson, Sandelman Software Works        | network architect  [ 
]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [ 
	

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 464 bytes --]

Re: netdevice notifier and device private data

[Alexander Aring]

Hi,

On Fri, Jun 08, 2018 at 03:37:44PM -0400, Michael Richardson wrote:
> 
> Alexander Aring <aring@mojatatu.com> wrote:
>     Alex> I already see code outside who changed tun netdevice to the
>     Alex> ARPHRD_6LOWPAN type and I suppose they running into this
>     Alex> issue.  (Btw: I don't know why somebody wants to changed that
>     Alex> type to ARPHRD_6LOWPAN on tun).
> 
> so that they can have the kernel do 6lowpan processing, emitting 6lowPAN
> packets into userspace to be transfered into a radio via some proprietary
> interface (including, for instance SLIP over USB cable to Contiki or OpenWSN stack, 
> set up to act as radio only)
> 

No, the datapath doesn't change. If the user space evaluate the
dev->type (there exists some ioctl() for it) it will assume it has a
6LoWPAN type interface.

A lot of user space software outside will doing interface specific
handling after detect the type. E.g. wireshark will select some dissector
handling.

On a tun interface and switch to 6LoWPAN it will not change much the
dissector view, because both raw IPv6 packets on datapath. For me as
6LoWPAN maintainer it makes no sense to switch to it. Currently
some netdevice notifier will crash (if you a lucky it will not).

Futhermore user space programs e.g. radvd will do 6lowpan specific
handling on 6lowpan dev->type, it will not work either on tun devices.

I know that wpantund from NestLabs do this switch, I am very curious
about the reason but I think they do it because the name is 6LoWPAN. But
wpantund is just a SLIP like protocol with additional radio/foo commands.

---

According to the people who say "I like to have a 6LoWPAN tun device,
that would be nice" - I don't know how this will ever work since 6LoWPAN
header highly depends on MAC header information. Tun devices works
because IP architecture allows a separation from MAC layer. I already
saw protocols at IETF where MAC header information are needed on top of
UDP payload in case of 6LoWPAN. (I talked about that at last netdev in
Montreal).

Bluetooth wanted to add a tun 6lowpan interface and I was curious how
this works. At the end it was a "Bluetooth mapping to ethernet header"
(not as tunnel, as propagated). I was not acking it, because if there
are protocols who needs more information than just what you can map to
ethernet... it will not work. At least it will also not work with IEEE
802.15.4 at all. They was just lucky that Bluetooth and ethernet use the
same mac address length (And I had some questions to the multicast bit
as well).

Tunnel might work to get mac information. But so far 6loWPAN works it is
that you have a L2 underlaying interface and on top (ip link set master)
a raw IPv6 interface which do the adaptation automatically as a protocol
translation (That's why I cannot understand Bluetooth 6LoWPAN use tx
queues on their 6LoWPAN interface, they need to fix the queue in the
underlaying L2 interface).

As an alternative solution I think it should be something done like TAP
like interface per subsystem. I mean NO ETHERTNET, but the ethernet in
TAP interfaces out and replace it with Bluetooth or IEEE 802.15.4.
I might can easily create a simple TAP IEEE 802.15.4 to show what I
mean.

With an IEEE 802.15.4 TAP device and a 6lowpan interface on top you can
realize your use case and pass 802.15.4 L2 frames to device node -> pops
up at 6LoPWAN interface and send IPv6 stuff and you can read on device
node.

I am still in the opinion the L2 TAP like interface is the way to go to
offer such feature.

- Alex

Re: netdevice notifier and device private data

[Michael Richardson]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]


Alexander Aring <aring@mojatatu.com> wrote:
    > Futhermore user space programs e.g. radvd will do 6lowpan specific
    > handling on 6lowpan dev->type, it will not work either on tun
    > devices.

    > I know that wpantund from NestLabs do this switch, I am very
    > curious about the reason but I think they do it because the name
    > is 6LoWPAN. But wpantund is just a SLIP like protocol with
    > additional radio/foo commands.

How do they change it then, and what does it do?
It totally seems like broken behaviour.  Maybe it's not even intentional.
Maybe they are just foobar.

-- 
]               Never tell me the odds!                 | ipv6 mesh networks [ 
]   Michael Richardson, Sandelman Software Works        | network architect  [ 
]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [ 
	

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 464 bytes --]

Re: netdevice notifier and device private data

[Alexander Aring]

Hi,

On Sat, Jun 09, 2018 at 03:01:18PM -0400, Michael Richardson wrote:
> 
> Alexander Aring <aring@mojatatu.com> wrote:
>     > Futhermore user space programs e.g. radvd will do 6lowpan specific
>     > handling on 6lowpan dev->type, it will not work either on tun
>     > devices.
> 
>     > I know that wpantund from NestLabs do this switch, I am very
>     > curious about the reason but I think they do it because the name
>     > is 6LoWPAN. But wpantund is just a SLIP like protocol with
>     > additional radio/foo commands.
> 
> How do they change it then, and what does it do?

They change it with the ioctl() of tun characte device, see [0].

What it does, it just changing the interface type to something else,
also there is no check at all that Linux has this interface type.

User space software e.g. radvd [1] will evaluate this type and doing
specific handling. Obviously changing it to 6LoWPAN and using this code
will confuse everything, because the handling makes only sense for a
6LoWPAN Linux interface which actually also use the 6LoWPAN subsystem.

They just using tun as all other to feed a IPv6 stack on a remote
microcontroller e.g. openthread, contiki, riot. via slip. (wpantund also
allow some radio, foo configuration).

> It totally seems like broken behaviour.  Maybe it's not even intentional.
> Maybe they are just foobar.
> 

They simple don't know what they doing... somebody thought 6LoWPAN need
to be 6LoWPAN, but they actually don't use the 6LoWPAN handling inside
the kernel. _Except_ they doing out of tree stuff which I don't believe.

According to [0] it also works with tun default (I suppsoe raw IPv6),
because ifdef. And they should not change it because they don't use
in-kernel 6LoWPAN functionality.

I really think that this tun/tap feature makes a lot of trouble for some
type changes. I probably introduce lowpan_dev pointer to netdevice and
then check if it's really a 6LoPWAN interface, a dev->type will not
garantuee anymore you have a 6LoWPAN interface. At least in user space
it's not possible to have a check if you really have a 6LoWPAN interface.

- Alex

[0] https://github.com/openthread/wpantund/blob/master/src/util/tunnel.c#L180
[1] https://github.com/reubenhwk/radvd/blob/master/device-linux.c#L75

Re: netdevice notifier and device private data

[Michael Richardson]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]


Alexander Aring <aring@mojatatu.com> wrote:
    >> It totally seems like broken behaviour.  Maybe it's not even
    >> intentional.  Maybe they are just foobar.

    > They simple don't know what they doing... somebody thought 6LoWPAN need
    > to be 6LoWPAN, but they actually don't use the 6LoWPAN handling inside
    > the kernel. _Except_ they doing out of tree stuff which I don't
    > believe.

So, it seems like this ioctl() should be disabled, or restricted to cases
that actually work.  hate to break their code, but if it's broken anyway, at
least the kernel won't crash under them.

    > According to [0] it also works with tun default (I suppsoe raw IPv6),
    > because ifdef. And they should not change it because they don't use
    > in-kernel 6LoWPAN functionality.

    > I really think that this tun/tap feature makes a lot of trouble for
    > some type changes. I probably introduce lowpan_dev pointer to netdevice
    > and then check if it's really a 6LoPWAN interface, a dev->type will not
    > garantuee anymore you have a 6LoWPAN interface. At least in user space
    > it's not possible to have a check if you really have a 6LoWPAN
    > interface.

    > - Alex

    > [0]
    > https://github.com/openthread/wpantund/blob/master/src/util/tunnel.c#L180
    > [1] https://github.com/reubenhwk/radvd/blob/master/device-linux.c#L75

-- 
Michael Richardson <mcr+IETF@sandelman.ca>, Sandelman Software Works
 -= IPv6 IoT consulting =-




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 464 bytes --]

Re: netdevice notifier and device private data

[Alexander Aring]

Hi,

On Sun, Jun 10, 2018 at 10:09:39PM -0400, Michael Richardson wrote:
> 
> Alexander Aring <aring@mojatatu.com> wrote:
>     >> It totally seems like broken behaviour.  Maybe it's not even
>     >> intentional.  Maybe they are just foobar.
> 
>     > They simple don't know what they doing... somebody thought 6LoWPAN need
>     > to be 6LoWPAN, but they actually don't use the 6LoWPAN handling inside
>     > the kernel. _Except_ they doing out of tree stuff which I don't
>     > believe.
> 
> So, it seems like this ioctl() should be disabled, or restricted to cases
> that actually work.  hate to break their code, but if it's broken anyway, at
> least the kernel won't crash under them.
> 

before we breaking their software I will gentle ask before why they
doing that and I get a good reason then. Then we look more how we deal
with an illegal read/dereference in dev->priv.

I will figure out how I can do that over github.

- Alex