Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net: l2: ieee802154: set the src addr before deciphering the frame #78525

Closed
wants to merge 1 commit into from
+13 −24
Closed

net: l2: ieee802154: set the src addr before deciphering the frame #78525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 12 additions & 21 deletions subsys/net/l2/ieee802154/ieee802154.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,9 @@ int ieee802154_radio_send(struct net_if *iface, struct net_pkt *pkt, struct net_
return -EIO;
}

static inline void swap_and_set_pkt_ll_addr(struct net_linkaddr *addr, bool has_pan_id,
enum ieee802154_addressing_mode mode,
struct ieee802154_address_field *ll)
static inline void set_pkt_ll_addr(struct net_linkaddr *addr, bool has_pan_id,
enum ieee802154_addressing_mode mode,
struct ieee802154_address_field *ll)
{
addr->type = NET_LINK_IEEE802154;

Expand All @@ -270,16 +270,6 @@ static inline void swap_and_set_pkt_ll_addr(struct net_linkaddr *addr, bool has_
addr->len = 0U;
addr->addr = NULL;
}

/* The net stack expects link layer addresses to be in
* big endian format for posix compliance so we must swap it.
* This is ok as the L2 address field comes from the header
* part of the packet buffer which will not be directly accessible
* once the packet reaches the upper layers.
*/
if (addr->len > 0) {
sys_mem_swap(addr->addr, addr->len);
}
}

/**
Expand Down Expand Up @@ -431,19 +421,20 @@ static enum net_verdict ieee802154_recv(struct net_if *iface, struct net_pkt *pk
return NET_OK;
}

set_pkt_ll_addr(net_pkt_lladdr_src(pkt), !fs->fc.pan_id_comp, fs->fc.src_addr_mode,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this definitely works, I think we should rather fix the L2/L3 encapsulation issue that caused this bug to slip through in the first place: IMO it would be better if we removed access to net_pkt_lladdr_src(pkt) (upper layers access to the LL addr) in ieee802154_decipher_data_frame() (L2) as it doesn't make sense to let an L2 function depend on mangled upper layer fields. I'd also prefer if fields being documented as "big endian" would not contain "little endian" data even temporarily. This has bitten us so many times already.

This means that setting and mangling the address fields for the upper layer packet should continue to be done last, i.e. just before we pass the packet on to L3.

(That being said: We'll still need access to the packet in ieee802154_decipher_data_frame() for the moment being to calculate L2 header length. But that can be removed in the future as well.)

mpdu.mhr.src_addr);

set_pkt_ll_addr(net_pkt_lladdr_dst(pkt), true, fs->fc.dst_addr_mode, mpdu.mhr.dst_addr);

if (!ieee802154_decipher_data_frame(iface, pkt, &mpdu)) {
return NET_DROP;
}

/* Setting L2 addresses must be done after packet authentication and internal
* packet handling as it will mangle the package header to comply with upper
* network layers' (POSIX) requirement to represent network addresses in big endian.
/* The net stack expects link layer addresses to be in
* big endian format for posix compliance so we must swap it.
*/
swap_and_set_pkt_ll_addr(net_pkt_lladdr_src(pkt), !fs->fc.pan_id_comp,
fs->fc.src_addr_mode, mpdu.mhr.src_addr);

swap_and_set_pkt_ll_addr(net_pkt_lladdr_dst(pkt), true, fs->fc.dst_addr_mode,
mpdu.mhr.dst_addr);
sys_mem_swap(net_pkt_lladdr_src(pkt)->addr, net_pkt_lladdr_src(pkt)->len);
sys_mem_swap(net_pkt_lladdr_dst(pkt)->addr, net_pkt_lladdr_dst(pkt)->len);

net_pkt_set_ll_proto_type(pkt, ETH_P_IEEE802154);

Expand Down
4 changes: 1 addition & 3 deletions subsys/net/l2/ieee802154/ieee802154_frame.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,6 @@ bool ieee802154_decipher_data_frame(struct net_if *iface, struct net_pkt *pkt,
uint8_t authtag_len = level_2_authtag_len[level];
uint8_t ll_hdr_len = (uint8_t *)mpdu->payload - net_pkt_data(pkt);
uint8_t payload_len = net_pkt_get_len(pkt) - ll_hdr_len - authtag_len;
uint8_t ext_addr_le[IEEE802154_EXT_ADDR_LENGTH];

/* TODO: Handle src short address.
* This will require to look up in nbr cache with short addr
Expand All @@ -970,9 +969,8 @@ bool ieee802154_decipher_data_frame(struct net_if *iface, struct net_pkt *pkt,
goto out;
}

sys_memcpy_swap(ext_addr_le, net_pkt_lladdr_src(pkt)->addr, net_pkt_lladdr_src(pkt)->len);
if (!ieee802154_decrypt_auth(&ctx->sec_ctx, net_pkt_data(pkt), ll_hdr_len, payload_len,
authtag_len, ext_addr_le,
authtag_len, net_pkt_lladdr_src(pkt)->addr,
sys_le32_to_cpu(mpdu->mhr.aux_sec->frame_counter))) {
NET_ERR("Could not decipher the frame");
goto out;
Expand Down
Loading