* [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL
@ 2019-07-01 15:33 Denis Kenzior
2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Denis Kenzior @ 2019-07-01 15:33 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Denis Kenzior
Commit 1c38c7f22068 ("nl80211: send event when CMD_FRAME duration
expires") added the possibility of NL80211_CMD_FRAME_WAIT_CANCEL
being sent whenever the off-channel wait time associated with a
CMD_FRAME completes. Document this in the uapi/linux/nl80211.h file.
Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
include/uapi/linux/nl80211.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Changes in v3:
- None
Changes in v2:
- update commit formatting
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8fc3a43cac75..0d9aad98c983 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -657,7 +657,9 @@
* is used during CSA period.
* @NL80211_CMD_FRAME_WAIT_CANCEL: When an off-channel TX was requested, this
* command may be used with the corresponding cookie to cancel the wait
- * time if it is known that it is no longer necessary.
+ * time if it is known that it is no longer necessary. This command is
+ * also sent as an event whenever the driver has completed the off-channel
+ * wait time.
* @NL80211_CMD_ACTION: Alias for @NL80211_CMD_FRAME for backward compatibility.
* @NL80211_CMD_FRAME_TX_STATUS: Report TX status of a management frame
* transmitted with %NL80211_CMD_FRAME. %NL80211_ATTR_COOKIE identifies
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 2/3] nl80211: Limit certain commands to interface owner 2019-07-01 15:33 [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior @ 2019-07-01 15:33 ` Denis Kenzior 2019-07-18 8:24 ` Arend Van Spriel 2019-07-31 9:51 ` Johannes Berg 2019-07-01 15:33 ` [PATCH v3 3/3] nl80211: Include wiphy address setup in NEW_WIPHY Denis Kenzior 2019-07-09 15:27 ` [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior 2 siblings, 2 replies; 8+ messages in thread From: Denis Kenzior @ 2019-07-01 15:33 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless, Denis Kenzior If the wdev object has been created (via NEW_INTERFACE) with SOCKET_OWNER attribute set, then limit certain commands only to the process that created that wdev. This can be used to make sure no other process on the system interferes by sending unwanted scans, action frames or any other funny business. This patch introduces a new internal flag, and checks that flag in the pre_doit hook. Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 19 deletions(-) Changes in v3: - Fix minor locking mistake reported by kernel test robot Changes in v2: - None diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index ff760ba83449..ebf5eab1f9b2 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -13587,6 +13587,7 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb, struct genl_info *info) #define NL80211_FLAG_NEED_WDEV_UP (NL80211_FLAG_NEED_WDEV |\ NL80211_FLAG_CHECK_NETDEV_UP) #define NL80211_FLAG_CLEAR_SKB 0x20 +#define NL80211_FLAG_OWNER_ONLY 0x40 static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb, struct genl_info *info) @@ -13595,6 +13596,7 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb, struct wireless_dev *wdev; struct net_device *dev; bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL; + int ret; if (rtnl) rtnl_lock(); @@ -13602,10 +13604,10 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb, if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) { rdev = cfg80211_get_dev_from_info(genl_info_net(info), info); if (IS_ERR(rdev)) { - if (rtnl) - rtnl_unlock(); - return PTR_ERR(rdev); + ret = PTR_ERR(rdev); + goto done; } + info->user_ptr[0] = rdev; } else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV || ops->internal_flags & NL80211_FLAG_NEED_WDEV) { @@ -13614,32 +13616,33 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb, wdev = __cfg80211_wdev_from_attrs(genl_info_net(info), info->attrs); if (IS_ERR(wdev)) { - if (rtnl) - rtnl_unlock(); - return PTR_ERR(wdev); + ret = PTR_ERR(wdev); + goto done; } dev = wdev->netdev; rdev = wiphy_to_rdev(wdev->wiphy); + ret = -EINVAL; if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) { - if (!dev) { - if (rtnl) - rtnl_unlock(); - return -EINVAL; - } + if (!dev) + goto done; info->user_ptr[1] = dev; } else { info->user_ptr[1] = wdev; } + ret = -ENETDOWN; if (ops->internal_flags & NL80211_FLAG_CHECK_NETDEV_UP && - !wdev_running(wdev)) { - if (rtnl) - rtnl_unlock(); - return -ENETDOWN; - } + !wdev_running(wdev)) + goto done; + + ret = -EPERM; + if (ops->internal_flags & NL80211_FLAG_OWNER_ONLY && + wdev->owner_nlportid && + wdev->owner_nlportid != info->snd_portid) + goto done; if (dev) dev_hold(dev); @@ -13647,7 +13650,13 @@ static int nl80211_pre_doit(const struct genl_ops *ops, struct sk_buff *skb, info->user_ptr[0] = rdev; } - return 0; + ret = 0; + +done: + if (rtnl && ret < 0) + rtnl_unlock(); + + return ret; } static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb, @@ -13712,7 +13721,8 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_set_interface, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV | - NL80211_FLAG_NEED_RTNL, + NL80211_FLAG_NEED_RTNL | + NL80211_FLAG_OWNER_ONLY, }, { .cmd = NL80211_CMD_NEW_INTERFACE, @@ -13728,7 +13738,8 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_del_interface, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV | - NL80211_FLAG_NEED_RTNL, + NL80211_FLAG_NEED_RTNL | + NL80211_FLAG_OWNER_ONLY, }, { .cmd = NL80211_CMD_GET_KEY, @@ -13745,6 +13756,7 @@ static const struct genl_ops nl80211_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | NL80211_FLAG_NEED_RTNL | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_CLEAR_SKB, }, { @@ -13754,6 +13766,7 @@ static const struct genl_ops nl80211_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | NL80211_FLAG_NEED_RTNL | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_CLEAR_SKB, }, { @@ -13762,6 +13775,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_del_key, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13778,6 +13792,7 @@ static const struct genl_ops nl80211_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .doit = nl80211_start_ap, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13786,6 +13801,7 @@ static const struct genl_ops nl80211_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .doit = nl80211_stop_ap, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13802,6 +13818,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_set_station, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13810,6 +13827,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_new_station, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13818,6 +13836,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_del_station, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13921,6 +13940,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_trigger_scan, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13929,6 +13949,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_abort_scan, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13942,6 +13963,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_start_sched_scan, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13950,6 +13972,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_stop_sched_scan, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13958,6 +13981,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_authenticate, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL | NL80211_FLAG_CLEAR_SKB, }, @@ -13967,6 +13991,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_associate, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL | NL80211_FLAG_CLEAR_SKB, }, @@ -13976,6 +14001,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_deauthenticate, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13984,6 +14010,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_disassociate, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -13992,6 +14019,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_join_ibss, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14000,6 +14028,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_leave_ibss, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, #ifdef CONFIG_NL80211_TESTMODE @@ -14019,6 +14048,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_connect, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL | NL80211_FLAG_CLEAR_SKB, }, @@ -14028,6 +14058,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_update_connect_params, .flags = GENL_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL | NL80211_FLAG_CLEAR_SKB, }, @@ -14037,6 +14068,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_disconnect, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14083,6 +14115,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_remain_on_channel, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14091,6 +14124,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_cancel_remain_on_channel, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14115,6 +14149,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_tx_mgmt, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14123,6 +14158,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_tx_mgmt_cancel_wait, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14147,6 +14183,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_set_cqm, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14221,6 +14258,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_set_rekey_data, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL | NL80211_FLAG_CLEAR_SKB, }, @@ -14278,6 +14316,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_start_p2p_device, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14286,6 +14325,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_stop_p2p_device, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14371,6 +14411,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_crit_protocol_start, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { @@ -14379,6 +14420,7 @@ static const struct genl_ops nl80211_ops[] = { .doit = nl80211_crit_protocol_stop, .flags = GENL_UNS_ADMIN_PERM, .internal_flags = NL80211_FLAG_NEED_WDEV_UP | + NL80211_FLAG_OWNER_ONLY | NL80211_FLAG_NEED_RTNL, }, { -- 2.21.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner 2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior @ 2019-07-18 8:24 ` Arend Van Spriel 2019-07-22 11:32 ` Denis Kenzior 2019-07-31 9:51 ` Johannes Berg 1 sibling, 1 reply; 8+ messages in thread From: Arend Van Spriel @ 2019-07-18 8:24 UTC (permalink / raw) To: Denis Kenzior, Johannes Berg; +Cc: linux-wireless On 7/1/2019 5:33 PM, Denis Kenzior wrote: > If the wdev object has been created (via NEW_INTERFACE) with > SOCKET_OWNER attribute set, then limit certain commands only to the > process that created that wdev. > > This can be used to make sure no other process on the system interferes > by sending unwanted scans, action frames or any other funny business. > > This patch introduces a new internal flag, and checks that flag in the > pre_doit hook. > > Signed-off-by: Denis Kenzior <denkenz@gmail.com> > --- > net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 61 insertions(+), 19 deletions(-) > > Changes in v3: > - Fix minor locking mistake reported by kernel test robot > > Changes in v2: > - None > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index ff760ba83449..ebf5eab1f9b2 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c [snip] > > - return 0; > + ret = 0; I suggest to keep the return 0 here for success path and only do the below for failure case (and obviously dropping '&& ret < 0'). Maybe rename label 'done' to 'fail' as well. > +done: > + if (rtnl && ret < 0) > + rtnl_unlock(); > + > + return ret; > } Regards, Arend ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner 2019-07-18 8:24 ` Arend Van Spriel @ 2019-07-22 11:32 ` Denis Kenzior 0 siblings, 0 replies; 8+ messages in thread From: Denis Kenzior @ 2019-07-22 11:32 UTC (permalink / raw) To: Arend Van Spriel, Johannes Berg; +Cc: linux-wireless Hi Arend, On 7/18/19 3:24 AM, Arend Van Spriel wrote: > On 7/1/2019 5:33 PM, Denis Kenzior wrote: >> If the wdev object has been created (via NEW_INTERFACE) with >> SOCKET_OWNER attribute set, then limit certain commands only to the >> process that created that wdev. >> >> This can be used to make sure no other process on the system interferes >> by sending unwanted scans, action frames or any other funny business. >> >> This patch introduces a new internal flag, and checks that flag in the >> pre_doit hook. >> >> Signed-off-by: Denis Kenzior <denkenz@gmail.com> >> --- >> net/wireless/nl80211.c | 80 ++++++++++++++++++++++++++++++++---------- >> 1 file changed, 61 insertions(+), 19 deletions(-) >> >> Changes in v3: >> - Fix minor locking mistake reported by kernel test robot >> >> Changes in v2: >> - None >> >> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c >> index ff760ba83449..ebf5eab1f9b2 100644 >> --- a/net/wireless/nl80211.c >> +++ b/net/wireless/nl80211.c > > [snip] > >> - return 0; >> + ret = 0; > > I suggest to keep the return 0 here for success path and only do the > below for failure case (and obviously dropping '&& ret < 0'). Maybe > rename label 'done' to 'fail' as well. > Sure, makes sense. I've made the suggested changes for v4. >> +done: >> + if (rtnl && ret < 0) >> + rtnl_unlock(); >> + >> + return ret; >> } > > Regards, > Arend Regards, -Denis ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner 2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior 2019-07-18 8:24 ` Arend Van Spriel @ 2019-07-31 9:51 ` Johannes Berg 2019-07-31 10:30 ` Denis Kenzior 1 sibling, 1 reply; 8+ messages in thread From: Johannes Berg @ 2019-07-31 9:51 UTC (permalink / raw) To: Denis Kenzior; +Cc: linux-wireless On Mon, 2019-07-01 at 10:33 -0500, Denis Kenzior wrote: > If the wdev object has been created (via NEW_INTERFACE) with > SOCKET_OWNER attribute set, then limit certain commands only to the > process that created that wdev. > > This can be used to make sure no other process on the system interferes > by sending unwanted scans, action frames or any other funny business. > > This patch introduces a new internal flag, and checks that flag in the > pre_doit hook. So, looking at this ... I can't say I'm convinced. You're tagging 35 out of about 106 commands, and even if a handful of those are new and were added after your patch, this doesn't really make sense. NL80211_CMD_LEAVE_IBSS is tagged, but not NL80211_CMD_LEAVE_MESH? NL80211_CMD_NEW_STATION is tagged, but not NL80211_CMD_NEW_MPATH? NL80211_CMD_SET_KEY is tagged, but not NL80211_CMD_SET_PMK or NL80211_CMD_SET_PMKSA? NL80211_CMD_UPDATE_CONNECT_PARAMS is tagged, but not NL80211_CMD_UPDATE_OWE_INFO (though this could be patch crossing?) NL80211_CMD_CONTROL_PORT_FRAME isn't tagged? NL80211_CMD_SET_QOS_MAP isn't tagged? It almost feels like you just did a "git grep NL80211_CMD_" on your code, and then dropped the flag on everything you were using. And honestly, I think you need a better justification than just "unwanted scans, action frames or any other funny business". Also, how's this not just a workaround for some very specific setup issue you were seeing, where people trying out iwd didn't remove wpa_s properly (*)? I'm really not convinced that this buys us anything except in very limited development scenarios - and those are typically the exact scenarios where you _want_ to be able to do things like that (and honestly, I'd be pretty pissed off if I couldn't do an "iw wlan0 scan" just because some tool decided it wanted to have control over things). (*) also, that would just happen to work for you now with iwd winning because you claim ownership and wpa_s doesn't, you'd still get the same complaints "iwd doesn't work" if/when wpa_s *does* start to claim ownership and you get locked out with a patch like this, so I don't feel you'd actually win much even in this case. I'm trying to come up with places where we do something similar, defend one application running as root against another ... but can't really? Think about VPN - we don't stop anying from removing or adding IP addresses that the VPN application didn't intend to use, yet that can obviously break your connection. You could even run dhcp on it, even if for (most) VPN protocols that's rather useless. Overall, I'm not really convinced. The design is rather unclear (randomly sprinkling magic dust on ~35% of commands), and it's also not really clear to me what this is intended to actually achieve. johannes ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] nl80211: Limit certain commands to interface owner 2019-07-31 9:51 ` Johannes Berg @ 2019-07-31 10:30 ` Denis Kenzior 0 siblings, 0 replies; 8+ messages in thread From: Denis Kenzior @ 2019-07-31 10:30 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless Hi Johannnes, On 7/31/19 4:51 AM, Johannes Berg wrote: > On Mon, 2019-07-01 at 10:33 -0500, Denis Kenzior wrote: >> If the wdev object has been created (via NEW_INTERFACE) with >> SOCKET_OWNER attribute set, then limit certain commands only to the >> process that created that wdev. >> >> This can be used to make sure no other process on the system interferes >> by sending unwanted scans, action frames or any other funny business. >> >> This patch introduces a new internal flag, and checks that flag in the >> pre_doit hook. > > So, looking at this ... > > I can't say I'm convinced. You're tagging 35 out of about 106 commands, > and even if a handful of those are new and were added after your patch, > this doesn't really make sense. > > NL80211_CMD_LEAVE_IBSS is tagged, but not NL80211_CMD_LEAVE_MESH? > NL80211_CMD_NEW_STATION is tagged, but not NL80211_CMD_NEW_MPATH? > NL80211_CMD_SET_KEY is tagged, but not NL80211_CMD_SET_PMK or > NL80211_CMD_SET_PMKSA? > NL80211_CMD_UPDATE_CONNECT_PARAMS is tagged, but not > NL80211_CMD_UPDATE_OWE_INFO (though this could be patch crossing?) > > NL80211_CMD_CONTROL_PORT_FRAME isn't tagged? So for some of these I was planning to submit a patch to check that the request comes from the SOCKET_OWNER for the connection itself. E.g. it makes no sense to accept CONTROL_PORT_FRAME from a process that didn't issue the CMD_CONNECT. Same applies for some of the offload commands. But I can include these in this list as well if you prefer. For others, it was pure ignorance as to what the commands do. E.g. we haven't looked into Mesh at all. So if you want to suggest which commands should be included, I'm happy to add these. > > NL80211_CMD_SET_QOS_MAP isn't tagged? > > It almost feels like you just did a "git grep NL80211_CMD_" on your > code, and then dropped the flag on everything you were using. > > And honestly, I think you need a better justification than just > "unwanted scans, action frames or any other funny business". > We have a limited resource that we are managing in userspace. We can't just have any random process coming in and messing with that resource. So either the userspace daemon should do it or the kernel. Right now it is just pure chaos... And really, in the end, how is this different from SOCKET_OWNER for CMD_CONNECT? It is optional in the end, so if you don't want to use it, don't? > Also, how's this not just a workaround for some very specific setup > issue you were seeing, where people trying out iwd didn't remove wpa_s > properly (*)? I'm really not convinced that this buys us anything except > in very limited development scenarios - and those are typically the > exact scenarios where you _want_ to be able to do things like that (and > honestly, I'd be pretty pissed off if I couldn't do an "iw wlan0 scan" > just because some tool decided it wanted to have control over things). I understand where you're coming from, but you're just one user who can disable this behavior anyway if you really cared. For 99.9% of the users this is never going to be a problem. And I'd rather cater to the 99%... > > (*) also, that would just happen to work for you now with iwd winning > because you claim ownership and wpa_s doesn't, you'd still get the same > complaints "iwd doesn't work" if/when wpa_s *does* start to claim > ownership and you get locked out with a patch like this, so I don't feel > you'd actually win much even in this case. > This is in no way the motivation for this. wpa_s or iwd winning is a distro/user configuration problem. I don't care about that now. Besides, this was mostly taken care of by the SOCKET_OWNER set on CMD_CONNECT... > > I'm trying to come up with places where we do something similar, defend > one application running as root against another ... but can't really? > Think about VPN - we don't stop anying from removing or adding IP > addresses that the VPN application didn't intend to use, yet that can > obviously break your connection. You could even run dhcp on it, even if > for (most) VPN protocols that's rather useless. File locking would be one example. Systemd can and does all kinds of fun stuff (e.g. locking a process out from twiddling rfkill). LSM modules can do just about everything. I think there are plenty of examples. But really, a lot of this works just because various processes play 'nice' and stay out of each other's way. Also, as you point out, most things aren't done because they don't make sense. But with nl80211 this isn't the case. Many processes would be tempted to start an operation or get some info out of nl80211 directly. So this patch tries to narrow down what they can use, e.g. informational-only commands are fine. Anything that can affect state is not fine. > > Overall, I'm not really convinced. The design is rather unclear > (randomly sprinkling magic dust on ~35% of commands), and it's also not > really clear to me what this is intended to actually achieve. > You may want to refer to the thread between Arend & Marcel (started by an earlier version of this patchset). I really don't see how giving the userspace management daemon (which by definition has exclusive control) a way from locking out a random process from starting a potentially disruptive operation is 'unconvincing.' As a developer you will hate this of course, and that is to be expected. But look at it from a user POV. Regards, -Denis ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] nl80211: Include wiphy address setup in NEW_WIPHY 2019-07-01 15:33 [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior 2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior @ 2019-07-01 15:33 ` Denis Kenzior 2019-07-09 15:27 ` [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior 2 siblings, 0 replies; 8+ messages in thread From: Denis Kenzior @ 2019-07-01 15:33 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless, Denis Kenzior Include wiphy address setup in wiphy dumps and new wiphy events. The wiphy permanent address is exposed as ATTR_MAC. If addr_mask is setup, then it is included as ATTR_MAC_MASK attribute. If multiple addresses are available, then their are exposed in a nested ATTR_MAC_ADDRS array. This information is already exposed via sysfs, but it makes sense to include it in the wiphy dump as well. Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- net/wireless/nl80211.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) Changes in v3: - None Changes in v2: - Move from case 0 to 9 diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index ebf5eab1f9b2..fb09c2301ed8 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -2166,6 +2166,31 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, rdev->wiphy.vht_capa_mod_mask)) goto nla_put_failure; + if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, + rdev->wiphy.perm_addr)) + goto nla_put_failure; + + if (!is_zero_ether_addr(rdev->wiphy.addr_mask) && + nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN, + rdev->wiphy.addr_mask)) + goto nla_put_failure; + + if (rdev->wiphy.n_addresses > 1) { + void *attr; + + attr = nla_nest_start_noflag(msg, + NL80211_ATTR_MAC_ADDRS); + if (!attr) + goto nla_put_failure; + + for (i = 0; i < rdev->wiphy.n_addresses; i++) + if (nla_put(msg, i + 1, ETH_ALEN, + rdev->wiphy.addresses[i].addr)) + goto nla_put_failure; + + nla_nest_end(msg, attr); + } + state->split_start++; break; case 10: -- 2.21.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL 2019-07-01 15:33 [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior 2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior 2019-07-01 15:33 ` [PATCH v3 3/3] nl80211: Include wiphy address setup in NEW_WIPHY Denis Kenzior @ 2019-07-09 15:27 ` Denis Kenzior 2 siblings, 0 replies; 8+ messages in thread From: Denis Kenzior @ 2019-07-09 15:27 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless ping? Regards, -Denis ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-07-31 10:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-01 15:33 [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior 2019-07-01 15:33 ` [PATCH v3 2/3] nl80211: Limit certain commands to interface owner Denis Kenzior 2019-07-18 8:24 ` Arend Van Spriel 2019-07-22 11:32 ` Denis Kenzior 2019-07-31 9:51 ` Johannes Berg 2019-07-31 10:30 ` Denis Kenzior 2019-07-01 15:33 ` [PATCH v3 3/3] nl80211: Include wiphy address setup in NEW_WIPHY Denis Kenzior 2019-07-09 15:27 ` [PATCH v3 1/3] nl80211: Update uapi for CMD_FRAME_WAIT_CANCEL Denis Kenzior
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).