netdev - Re: [PATCH wpan-next v4 07/11] mac802154: Handle association requests from peers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK-6q+iTOapHF7ftqtRQBsNUYEKqjS0Mkq4O-A2C2tbupStk0A@mail.gmail.com>
Date: Sun, 24 Sep 2023 20:13:34 -0400
From: Alexander Aring <aahringo@...hat.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Alexander Aring <alex.aring@...il.com>, Stefan Schmidt <stefan@...enfreihafen.org>, 
	linux-wpan@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, 
	netdev@...r.kernel.org, David Girault <david.girault@...vo.com>, 
	Romuald Despres <romuald.despres@...vo.com>, Frederic Blain <frederic.blain@...vo.com>, 
	Nicolas Schodet <nico@...fr.eu.org>, Guilhem Imberton <guilhem.imberton@...vo.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next v4 07/11] mac802154: Handle association requests
 from peers

Hi,

On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal
<miquel.raynal@...tlin.com> wrote:
>
> Coordinators may have to handle association requests from peers which
> want to join the PAN. The logic involves:
> - Acknowledging the request (done by hardware)
> - If requested, a random short address that is free on this PAN should
>   be chosen for the device.
> - Sending an association response with the short address allocated for
>   the peer and expecting it to be ack'ed.
>
> If anything fails during this procedure, the peer is considered not
> associated.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> ---
>  include/net/cfg802154.h         |   7 ++
>  include/net/ieee802154_netdev.h |   6 ++
>  net/ieee802154/core.c           |   7 ++
>  net/ieee802154/pan.c            |  30 +++++++
>  net/mac802154/ieee802154_i.h    |   2 +
>  net/mac802154/rx.c              |   8 ++
>  net/mac802154/scan.c            | 142 ++++++++++++++++++++++++++++++++
>  7 files changed, 202 insertions(+)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 9b036ab20079..c844ae63bc04 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -583,4 +583,11 @@ struct ieee802154_pan_device *
>  cfg802154_device_is_child(struct wpan_dev *wpan_dev,
>                           struct ieee802154_addr *target);
>
> +/**
> + * cfg802154_get_free_short_addr - Get a free address among the known devices
> + * @wpan_dev: the wpan device
> + * @return: a random short address expectedly unused on our PAN
> + */
> +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev);
> +
>  #endif /* __NET_CFG802154_H */
> diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> index 16194356cfe7..4de858f9929e 100644
> --- a/include/net/ieee802154_netdev.h
> +++ b/include/net/ieee802154_netdev.h
> @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame {
>         struct ieee802154_assoc_req_pl assoc_req_pl;
>  };
>
> +struct ieee802154_association_resp_frame {
> +       struct ieee802154_hdr mhr;
> +       struct ieee802154_mac_cmd_pl mac_pl;
> +       struct ieee802154_assoc_resp_pl assoc_resp_pl;
> +};
> +
>  struct ieee802154_disassociation_notif_frame {
>         struct ieee802154_hdr mhr;
>         struct ieee802154_mac_cmd_pl mac_pl;
> diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
> index a08d75dd56ad..1670a71327a7 100644
> --- a/net/ieee802154/core.c
> +++ b/net/ieee802154/core.c
> @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free);
>
>  static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev)
>  {
> +       struct ieee802154_pan_device *child, *tmp;
> +
>         mutex_lock(&wpan_dev->association_lock);
>
>         kfree(wpan_dev->parent);
>         wpan_dev->parent = NULL;
>
> +       list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) {
> +               list_del(&child->node);
> +               kfree(child);
> +       }
> +
>         mutex_unlock(&wpan_dev->association_lock);
>  }
>
> diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
> index 9e1f1973c294..e99c64054dcb 100644
> --- a/net/ieee802154/pan.c
> +++ b/net/ieee802154/pan.c
> @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev,
>         return NULL;
>  }
>  EXPORT_SYMBOL_GPL(cfg802154_device_is_child);
> +
> +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev)
> +{
> +       struct ieee802154_pan_device *child;
> +       __le16 addr;
> +
> +       lockdep_assert_held(&wpan_dev->association_lock);
> +
> +       do {
> +               get_random_bytes(&addr, 2);
> +               if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) ||
> +                   addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC))
> +                       continue;
> +
> +               if (wpan_dev->short_addr == addr)
> +                       continue;
> +
> +               if (wpan_dev->parent && wpan_dev->parent->short_addr == addr)
> +                       continue;
> +
> +               list_for_each_entry(child, &wpan_dev->children, node)
> +                       if (child->short_addr == addr)
> +                               continue;
> +
> +               break;
> +       } while (1);
> +

I still believe that this random 2 bytes and check if it's already
being used is wrong here. We need something to use the next free
available number according to the data we are storing here.

However it is acceptable and can be changed later...

- Alex