On 8/26/2022 5:55 AM, Vinayak Yadawad wrote:
if this is a v2 it should have been annotated as such with a change log
> In case of 4way handshake offload, transition disable policy
> updated by the AP during EAPOL 3/4 is not updated to the upper layer.
> This results in mismatch between transition disable policy
> between the upper layer and the driver. This patch addresses this
> issue by updating transition disable policy as part of port
> authorization indiation.
s/indiation/indication/
>
> Signed-off-by: Vinayak Yadawad <[email protected]>
> ---
> .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
> include/net/cfg80211.h | 3 ++-
> include/uapi/linux/nl80211.h | 3 +++
> net/wireless/core.h | 4 +++-
> net/wireless/nl80211.c | 7 ++++++-
> net/wireless/nl80211.h | 3 ++-
> net/wireless/sme.c | 8 +++++---
> net/wireless/util.c | 2 +-
> 8 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index db45da33adfd..6654c4167c36 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -6028,7 +6028,7 @@ brcmf_bss_roaming_done(struct brcmf_cfg80211_info *cfg,
> brcmf_dbg(CONN, "Report roaming result\n");
>
> if (profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X && profile->is_ft) {
> - cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
> + cfg80211_port_authorized(ndev, profile->bssid, -1, GFP_KERNEL);
> brcmf_dbg(CONN, "Report port authorized\n");
> }
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 908d58393484..2fc173e88d31 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -7659,6 +7659,7 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
> *
> * @dev: network device
> * @bssid: the BSSID of the AP
> + * @td_bitmap: transition disable policy
> * @gfp: allocation flags
> *
> * This function should be called by a driver that supports 4 way handshake
> @@ -7669,7 +7670,7 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
> * indicate the 802.11 association.
> */
> void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
> - gfp_t gfp);
> + s16 td_bitmap, gfp_t gfp);
>
> /**
> * cfg80211_disconnected - notify cfg80211 that connection was dropped
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index ffb7c573e299..c81cdc6d6c86 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -2741,6 +2741,8 @@ enum nl80211_commands {
> * When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
> * timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
> * the incoming frame RX timestamp.
> + * @NL80211_ATTR_TD_BITMAP: Transition Disable bitmap, for subsequent
> + * (re)associations.
> * @NUM_NL80211_ATTR: total number of nl80211_attrs available
> * @NL80211_ATTR_MAX: highest attribute number currently defined
> * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -3268,6 +3270,7 @@ enum nl80211_attrs {
>
> NL80211_ATTR_TX_HW_TIMESTAMP,
> NL80211_ATTR_RX_HW_TIMESTAMP,
> + NL80211_ATTR_TD_BITMAP,
>
> /* add attributes here, update the policy in nl80211.c */
Johannes, do you want the policy updated even thought this is
driver->userspace and hence the policy is never applied to it?
>
> diff --git a/net/wireless/core.h b/net/wireless/core.h
> index 775e16cb99ed..d36d4b775284 100644
> --- a/net/wireless/core.h
> +++ b/net/wireless/core.h
> @@ -271,6 +271,7 @@ struct cfg80211_event {
> } ij;
> struct {
> u8 bssid[ETH_ALEN];
> + s16 td_bitmap;
I know you are using -1 as an indication that the bitmap is not used,
but using signed with a bitmap seems strange since bitops can be
affected by sign extension. Just something that set off my Spider-Sense.
> } pa;
> };
> };
> @@ -409,7 +410,8 @@ int cfg80211_disconnect(struct cfg80211_registered_device *rdev,
> bool wextev);
> void __cfg80211_roamed(struct wireless_dev *wdev,
> struct cfg80211_roam_info *info);
> -void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid);
> +void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid,
> + s16 td_bitmap);
> int cfg80211_mgd_wext_connect(struct cfg80211_registered_device *rdev,
> struct wireless_dev *wdev);
> void cfg80211_autodisconnect_wk(struct work_struct *work);
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2705e3ee8fc4..176aa7b3bc6c 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -17841,7 +17841,8 @@ void nl80211_send_roamed(struct cfg80211_registered_device *rdev,
> }
>
> void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
> - struct net_device *netdev, const u8 *bssid)
> + struct net_device *netdev, const u8 *bssid,
> + s16 td_bitmap)
> {
> struct sk_buff *msg;
> void *hdr;
> @@ -17861,6 +17862,10 @@ void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
> nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, bssid))
> goto nla_put_failure;
>
> + if (td_bitmap >= 0)
> + if (nla_put_s16(msg, NL80211_ATTR_TD_BITMAP, td_bitmap))
> + goto nla_put_failure;
> +
> genlmsg_end(msg, hdr);
>
> genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
> diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
> index 855d540ddfb9..17abb0a46d8c 100644
> --- a/net/wireless/nl80211.h
> +++ b/net/wireless/nl80211.h
> @@ -83,7 +83,8 @@ void nl80211_send_roamed(struct cfg80211_registered_device *rdev,
> struct net_device *netdev,
> struct cfg80211_roam_info *info, gfp_t gfp);
> void nl80211_send_port_authorized(struct cfg80211_registered_device *rdev,
> - struct net_device *netdev, const u8 *bssid);
> + struct net_device *netdev, const u8 *bssid,
> + s16 td_bitmap);
> void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
> struct net_device *netdev, u16 reason,
> const u8 *ie, size_t ie_len, bool from_ap);
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 27fb2a0c4052..e3150b9edb88 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -1234,7 +1234,8 @@ void cfg80211_roamed(struct net_device *dev, struct cfg80211_roam_info *info,
> }
> EXPORT_SYMBOL(cfg80211_roamed);
>
> -void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid)
> +void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid,
> + s16 td_bitmap)
> {
> ASSERT_WDEV_LOCK(wdev);
>
> @@ -1247,11 +1248,11 @@ void __cfg80211_port_authorized(struct wireless_dev *wdev, const u8 *bssid)
> return;
>
> nl80211_send_port_authorized(wiphy_to_rdev(wdev->wiphy), wdev->netdev,
> - bssid);
> + bssid, td_bitmap);
> }
>
> void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
> - gfp_t gfp)
> + s16 td_bitmap, gfp_t gfp)
> {
> struct wireless_dev *wdev = dev->ieee80211_ptr;
> struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
> @@ -1267,6 +1268,7 @@ void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
>
> ev->type = EVENT_PORT_AUTHORIZED;
> memcpy(ev->pa.bssid, bssid, ETH_ALEN);
> + ev->pa.td_bitmap = td_bitmap;
>
> /*
> * Use the wdev event list so that if there are pending
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 2c127951764a..6a1d82c43766 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -988,7 +988,7 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
> __cfg80211_leave(wiphy_to_rdev(wdev->wiphy), wdev);
> break;
> case EVENT_PORT_AUTHORIZED:
> - __cfg80211_port_authorized(wdev, ev->pa.bssid);
> + __cfg80211_port_authorized(wdev, ev->pa.bssid, ev->pa.td_bitmap);
> break;
> }
> wdev_unlock(wdev);
On Fri, 2022-08-26 at 08:48 -0700, Jeff Johnson wrote:
>
> > @@ -3268,6 +3270,7 @@ enum nl80211_attrs {
> >
> > NL80211_ATTR_TX_HW_TIMESTAMP,
> > NL80211_ATTR_RX_HW_TIMESTAMP,
> > + NL80211_ATTR_TD_BITMAP,
> >
> > /* add attributes here, update the policy in nl80211.c */
>
> Johannes, do you want the policy updated even thought this is
> driver->userspace and hence the policy is never applied to it?
Yeah in a sense, it doesn't really matter... I think not updating is
fine, then it will likely even be rejected, at least in any new
commands.
> > struct {
> > u8 bssid[ETH_ALEN];
> > + s16 td_bitmap;
>
> I know you are using -1 as an indication that the bitmap is not used,
> but using signed with a bitmap seems strange since bitops can be
> affected by sign extension. Just something that set off my Spider-Sense.
Yeah true ... maybe a separate validity bool would've been better?
dunno.
johannes
Hi Jeff, Johannes,
From your points we understand, we need a length variable and the
td_bitmap value.
As per the spec
https://www.wi-fi.org/download.php?file=/sites/default/files/private/WPA3_Specification_v3.0.pdf(
Table 4. Transition Disable KDE format),
we do have variable length of the bitmap. So we could
1. Add 2 arguments
2. One would be for the length of td_bitmap.
3. The second argument would be an u8 bitmap array depending on length
of bitmap.
Accordingly checks can be added to indicate/ignore the indication of
bitmap to upper layer.
Also the driver can update these fields as length 0 and array NULL in
case no value to be updated by the driver.
On Mon, Aug 29, 2022 at 3:22 PM Johannes Berg <[email protected]> wrote:
>
> On Fri, 2022-08-26 at 08:48 -0700, Jeff Johnson wrote:
> >
> > > @@ -3268,6 +3270,7 @@ enum nl80211_attrs {
> > >
> > > NL80211_ATTR_TX_HW_TIMESTAMP,
> > > NL80211_ATTR_RX_HW_TIMESTAMP,
> > > + NL80211_ATTR_TD_BITMAP,
> > >
> > > /* add attributes here, update the policy in nl80211.c */
> >
> > Johannes, do you want the policy updated even thought this is
> > driver->userspace and hence the policy is never applied to it?
>
> Yeah in a sense, it doesn't really matter... I think not updating is
> fine, then it will likely even be rejected, at least in any new
> commands.
>
> > > struct {
> > > u8 bssid[ETH_ALEN];
> > > + s16 td_bitmap;
> >
> > I know you are using -1 as an indication that the bitmap is not used,
> > but using signed with a bitmap seems strange since bitops can be
> > affected by sign extension. Just something that set off my Spider-Sense.
>
> Yeah true ... maybe a separate validity bool would've been better?
> dunno.
>
> johannes
>
Hi Johannes, Jeff,
I have published the version2 with the above points mentioned.
[PATCHv2,1/1] cfg80211: Update Transition Disable policy during port
authorization
On Tue, Aug 30, 2022 at 2:46 PM Vinayak Yadawad
<[email protected]> wrote:
>
> Hi Jeff, Johannes,
>
> From your points we understand, we need a length variable and the
> td_bitmap value.
>
> As per the spec
> https://www.wi-fi.org/download.php?file=/sites/default/files/private/WPA3_Specification_v3.0.pdf(
> Table 4. Transition Disable KDE format),
> we do have variable length of the bitmap. So we could
> 1. Add 2 arguments
> 2. One would be for the length of td_bitmap.
> 3. The second argument would be an u8 bitmap array depending on length
> of bitmap.
> Accordingly checks can be added to indicate/ignore the indication of
> bitmap to upper layer.
> Also the driver can update these fields as length 0 and array NULL in
> case no value to be updated by the driver.
>
> On Mon, Aug 29, 2022 at 3:22 PM Johannes Berg <[email protected]> wrote:
> >
> > On Fri, 2022-08-26 at 08:48 -0700, Jeff Johnson wrote:
> > >
> > > > @@ -3268,6 +3270,7 @@ enum nl80211_attrs {
> > > >
> > > > NL80211_ATTR_TX_HW_TIMESTAMP,
> > > > NL80211_ATTR_RX_HW_TIMESTAMP,
> > > > + NL80211_ATTR_TD_BITMAP,
> > > >
> > > > /* add attributes here, update the policy in nl80211.c */
> > >
> > > Johannes, do you want the policy updated even thought this is
> > > driver->userspace and hence the policy is never applied to it?
> >
> > Yeah in a sense, it doesn't really matter... I think not updating is
> > fine, then it will likely even be rejected, at least in any new
> > commands.
> >
> > > > struct {
> > > > u8 bssid[ETH_ALEN];
> > > > + s16 td_bitmap;
> > >
> > > I know you are using -1 as an indication that the bitmap is not used,
> > > but using signed with a bitmap seems strange since bitops can be
> > > affected by sign extension. Just something that set off my Spider-Sense.
> >
> > Yeah true ... maybe a separate validity bool would've been better?
> > dunno.
> >
> > johannes
> >
On Fri, 2022-08-26 at 18:25 +0530, Vinayak Yadawad wrote:
>
> @@ -1267,6 +1268,7 @@ void cfg80211_port_authorized(struct net_device *dev, const u8 *bssid,
>
> ev->type = EVENT_PORT_AUTHORIZED;
> memcpy(ev->pa.bssid, bssid, ETH_ALEN);
> + ev->pa.td_bitmap = td_bitmap;
>
>
Surely this will cause some kind of use-after-free, or stack use after
stack frame return??
In the event, I guess you need to size it for the max possible bitmap
size and copy it.
(also nit somewhere: "u8 *x" instead of "u8* x")
The function argument should probably also be const.
FWIW, I didn't really mind having a fixed two-byte bitmap, but that
doesn't address the case of it being not valid.
We could just use an "int" and say "-1 for invalid, otherwise a 16-bit
bitmap value"?
johannes