[PATCH] nl80211: Don't verify owner_nlportid on NAN commands

[PATCH] nl80211: Don't verify owner_nlportid on NAN commands

[Luca Coelho]

From: Andrei Otcheretianski <andrei.otcheretianski@intel.com>

If NAN interface is created with NL80211_ATTR_SOCKET_OWNER, the socket
that is used to create the interface is used for all NAN operations and
reporting NAN events.
However, it turns out that sending commands and receiving events on
the same socket is not possible in a completely race-free way:
If the socket buffer is overflowed by the events, the command response
will not be sent. In that case the caller will block forever on recv.
Using non-blocking socket for commands is more complicated and still
the command response or ack may not be received.
So, keep unicasting NAN events to the interface creator, but allow
using a different socket for commands.

Signed-off-by: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 net/wireless/nl80211.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5487cd775b6f..45ba3d0872cc 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -11206,10 +11206,6 @@ static int nl80211_nan_add_func(struct sk_buff *skb,
 	if (!info->attrs[NL80211_ATTR_NAN_FUNC])
 		return -EINVAL;
 
-	if (wdev->owner_nlportid &&
-	    wdev->owner_nlportid != info->snd_portid)
-		return -ENOTCONN;
-
 	err = nla_parse_nested(tb, NL80211_NAN_FUNC_ATTR_MAX,
 			       info->attrs[NL80211_ATTR_NAN_FUNC],
 			       nl80211_nan_func_policy, info->extack);
@@ -11441,10 +11437,6 @@ static int nl80211_nan_del_func(struct sk_buff *skb,
 	if (!info->attrs[NL80211_ATTR_COOKIE])
 		return -EINVAL;
 
-	if (wdev->owner_nlportid &&
-	    wdev->owner_nlportid != info->snd_portid)
-		return -ENOTCONN;
-
 	cookie = nla_get_u64(info->attrs[NL80211_ATTR_COOKIE]);
 
 	rdev_del_nan_func(rdev, wdev, cookie);
-- 
2.11.0

Re: [PATCH] nl80211: Don't verify owner_nlportid on NAN commands

[Arend van Spriel]

On 23-06-17 11:26, Luca Coelho wrote:
> From: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
> 
> If NAN interface is created with NL80211_ATTR_SOCKET_OWNER, the socket
> that is used to create the interface is used for all NAN operations and
> reporting NAN events.
> However, it turns out that sending commands and receiving events on
> the same socket is not possible in a completely race-free way:
> If the socket buffer is overflowed by the events, the command response
> will not be sent. In that case the caller will block forever on recv.
> Using non-blocking socket for commands is more complicated and still
> the command response or ack may not be received.
> So, keep unicasting NAN events to the interface creator, but allow
> using a different socket for commands.

Sounds like this patch is missing significant changes in the
documentation of nl80211 API:

 * @NL80211_CMD_ADD_NAN_FUNCTION: Add a NAN function. The function is
defined
 *      with %NL80211_ATTR_NAN_FUNC nested attribute. When called, this
 *      operation returns the strictly positive and unique instance id
 *      (%NL80211_ATTR_NAN_FUNC_INST_ID) and a cookie (%NL80211_ATTR_COOKIE)
 *      of the function upon success.
 *      Since instance ID's can be re-used, this cookie is the right
 *      way to identify the function. This will avoid races when a
termination
 *      event is handled by the user space after it has already added a new
 *      function that got the same instance id from the kernel as the one
 *      which just terminated.
 *      This cookie may be used in NAN events even before the command
 *      returns, so userspace shouldn't process NAN events until it
processes
 *      the response to this command.
 *      Look at %NL80211_ATTR_SOCKET_OWNER as well.

/and/

 * @NL80211_ATTR_SOCKET_OWNER: Flag attribute, if set during interface

[...]

 *      If set during NAN interface creation, the interface will be
destroyed
 *      if the socket is closed just like any other interface. Moreover,
only
 *      the netlink socket that created the interface will be allowed to add
 *      and remove functions. NAN notifications will be sent in unicast
to that
 *      socket. Without this attribute, any socket can add functions and the
 *      notifications will be sent to the %NL80211_MCGRP_NAN multicast
group.

Also is this not a more fundamental flaw in netlink socket behavior.
Should there be some priority imposed on command responses over event
messages. Just seems like this patch is a workaround.

Regards,
Arend

Re: [PATCH] nl80211: Don't verify owner_nlportid on NAN commands

[Johannes Berg]

On Fri, 2017-06-23 at 23:03 +0200, Arend van Spriel wrote:
> 
> Also is this not a more fundamental flaw in netlink socket behavior.
> Should there be some priority imposed on command responses over event
> messages. Just seems like this patch is a workaround.

In a way, but it's not that easy. ACK messages are really just that,
messages, and having them bypass the queue might not be reasonable for
the use case, having them be queued unconditionally would open an
avenue for memory consumption attacks (send lots of dummy commands and
let the ACKs accumulate), and deleting already "delivered" message
could, I think, break semantics because I think delivery can be checked
by the sender?

Either way, I don't really see a good way to solve this problem in
netlink.

johannes

Re: [PATCH] nl80211: Don't verify owner_nlportid on NAN commands

[Luca Coelho]

On Fri, 2017-06-23 at 23:03 +0200, Arend van Spriel wrote:
> On 23-06-17 11:26, Luca Coelho wrote:
> > From: Andrei Otcheretianski <andrei.otcheretianski@intel.com>
> > 
> > If NAN interface is created with NL80211_ATTR_SOCKET_OWNER, the socket
> > that is used to create the interface is used for all NAN operations and
> > reporting NAN events.
> > However, it turns out that sending commands and receiving events on
> > the same socket is not possible in a completely race-free way:
> > If the socket buffer is overflowed by the events, the command response
> > will not be sent. In that case the caller will block forever on recv.
> > Using non-blocking socket for commands is more complicated and still
> > the command response or ack may not be received.
> > So, keep unicasting NAN events to the interface creator, but allow
> > using a different socket for commands.
> 
> Sounds like this patch is missing significant changes in the
> documentation of nl80211 API:

Thanks Arend!

Andrei is going to fix the documentation and I'll send v2.


--
Luca.