Re: [RFC PATCH net-next 7/7] net: airoha: add SOE XFRM packet offload support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Asynchronous XFRM state GC causes a Use-After-Free of the conduit network device private data when an SA is offloaded via a DSA user port.
- [Critical] Forcefully clearing `sa->inflight` during SA teardown prematurely signals completion, causing packet misrouting and context UAF.
- [High] DSA proxy scan hardcodes `init_net`, causing `xfrmdev_ops` to leak on interfaces moved to other network namespaces.
- [High] Hardware-decrypted packets are injected into an Ethernet device via `netif_rx()` without a valid MAC header.
- [High] Out-of-place TX offload loses critical SKB metadata, breaking QoS and netfilter marks.
- [High] Cleared Security Associations remain active in hardware because `AIROHA_SOE_SA_CMD_VLD` is unconditionally set.
- [High] The `xdo_dev_state_free` callback performs sleeping operations, causing a kernel BUG when the XFRM state is released in softirq context.
- [High] RX offload is unconditionally broken for DSA user ports due to a strict `x->xso.dev != rx_dev` check and lack of DSA tag parsing.
- [High] Unverified RX packets have their protocol unconditionally overwritten and are irrevocably truncated, corrupting non-IPv4 traffic.
- [High] TX offload expects `skb->data` to point to the IP header, but it points to the MAC header, causing TX offload failure.
- [Medium] Inefficient DSA proxy scanning causes redundant O(N^2) netdev feature updates system-wide.
--

commit c8afd29ed4385ffd75d9f77b7cea01b7c16511f3
Author: Jihong Min <hurryman2212@gmail.com>
Subject: net: airoha: add SOE XFRM packet offload support

Add the EN7581 Secure Offload Engine provider. The provider programs ESP
SAs, exposes NETIF_F_HW_ESP through xfrmdev_ops, submits encrypt and
decrypt packets through the QDMA SOE path, and handles SOE completion
delivery.

> diff --git a/drivers/net/ethernet/airoha/airoha_soe.c b/drivers/net/ethernet/airoha/airoha_soe.c
> new file mode 100644
> index 0000000000000..3a240ed44d7fc
> --- /dev/null
> +++ b/drivers/net/ethernet/airoha/airoha_soe.c
[ ... ]
> +static int airoha_soe_program_sa_locked(struct airoha_soe *soe,
> +					unsigned int index,
> +					const struct airoha_soe_sa_cfg *cfg)
> +{
> +	void __iomem *base = soe->base;
> +
> +	writel(cfg->cmd | AIROHA_SOE_SA_CMD_VLD, base + AIROHA_SOE_SA_CMD);

[Severity: High]
Does the unconditional use of AIROHA_SOE_SA_CMD_VLD defeat the teardown
zeroing?

When an XFRM state is destroyed, airoha_soe_clear_sa_locked() attempts to
invalidate the slot by passing a zero-initialized airoha_soe_sa_cfg, but
this unconditionally applies the VLD bit before writing to the hardware
command register.

Could this leave the hardware SA slot active with an SPI of 0, allowing a
malicious packet carrying SPI 0 to match this slot and bypass security
checks?

[ ... ]
> +static void airoha_soe_abort_sa(struct airoha_soe_sa *sa)
> +{
> +	LIST_HEAD(rx_queue);
> +	LIST_HEAD(tx_queue);
> +
> +	if (!sa)
> +		return;
> +
> +	spin_lock_bh(&sa->lock);
> +	sa->dead = true;
> +	airoha_soe_forget_rx_ctx_list(sa);
> +	list_splice_init(&sa->tx_queue, &tx_queue);
> +	list_splice_init(&sa->rx_queue, &rx_queue);
> +	sa->inflight = 0;
> +	complete(&sa->idle);
> +	spin_unlock_bh(&sa->lock);

[Severity: Critical]
Does forcefully zeroing sa->inflight and signaling sa->idle destroy the
synchronization accounting while packets are still in hardware?

During XFRM state deletion, airoha_soe_abort_sa() breaks the synchronization
in airoha_soe_free_sa(), causing the wait timeout to return instantly. The
SA index is then reallocated to a new XFRM state while old packets are
still in flight.

When the hardware delivers the completion, airoha_soe_rx_skb() pops a
tracking context from the new SA's queue based solely on the reused index,
which applies incorrect XFRM states and corrupts context accounting.

[ ... ]
> +static void airoha_soe_free_sa(struct airoha_soe_sa *sa)
> +{
> +	LIST_HEAD(rx_queue);
> +	LIST_HEAD(tx_queue);
> +	struct airoha_soe *soe;
> +
> +	if (!sa)
> +		return;
> +
> +	soe = sa->soe;
> +	airoha_soe_mark_sa_dead(sa);
> +	if (!wait_for_completion_timeout(&sa->idle, AIROHA_SOE_SA_FREE_TIMEOUT))
> +		dev_warn(soe->dev,

[Severity: High]
Is it safe to perform sleeping operations during XFRM state destruction?

The xdo_dev_state_free callback is executed synchronously from
__xfrm_state_destroy, which can be invoked in atomic or softirq context
when the last reference is dropped. This often happens when a packet is
dropped in the RX path and the network stack releases its routing entry.

Since airoha_soe_free_sa() waits for a completion and calls
synchronize_rcu(), executing these in softirq context would cause a
scheduling while atomic BUG.

[ ... ]
> +static int airoha_soe_prepare_ip_headers(struct sk_buff *skb)
> +{
> +	unsigned int hdr_len;
> +
> +	if (!pskb_may_pull(skb, 1))
> +		return -EINVAL;
> +
> +	switch (skb->data[0] & 0xf0) {
> +	case 0x40:
> +		hdr_len = sizeof(struct iphdr);

[Severity: High]
Will this incorrectly read the first byte of the MAC header instead of the
IP version field?

The airoha_soe_xmit() function serves as the xdo_dev_packet_xmit callback,
which is invoked by the core network stack immediately before handing the
packet to the driver's transmit routine. At this stage, the MAC header has
been added and the data pointer points to the Ethernet MAC header.

Reading the first byte checks the destination MAC address instead, causing
offload to randomly fail and drop packets unless the destination MAC
happens to start with 0x4 or 0x6.

[ ... ]
> +static bool airoha_soe_rx_parse_ipv4(struct sk_buff *skb,
> +				     struct airoha_soe_rx_info *info)
> +{
[ ... ]
> +	if (skb->protocol != htons(ETH_P_IP)) {
> +		if (!pskb_may_pull(skb, 1) || (skb->data[0] >> 4) != 4)
> +			return false;
> +
> +		skb->protocol = htons(ETH_P_IP);
> +	}
[ ... ]
> +bool airoha_soe_rx_plain_skb(struct airoha_gdm_dev *dev, struct sk_buff *skb,
> +			     struct net_device *rx_dev, u16 foe_hash,
> +			     u32 foe_reason, bool foe_valid)
> +{
[ ... ]
> +	skb_reset_network_header(skb);
> +	if (!airoha_soe_rx_parse_ipv4(skb, &info))
> +		return false;
> +
> +	if (skb->len != info.packet_len && pskb_trim(skb, info.packet_len))
> +		return false;

[Severity: High]
Can this corrupt non-IPv4 traffic by irrevocably truncating the packet?

In airoha_soe_rx_plain_skb(), the protocol is overwritten and the packet is
truncated via pskb_trim() before verifying that the packet belongs to an
offloaded ESP SA.

If a non-IPv4 packet has 0x4 in the upper nibble of its first byte,
airoha_soe_rx_parse_ipv4() treats it as IPv4, modifies the protocol, and
truncates it. If xfrm_input_state_lookup() later fails, the corrupted
packet is returned to the network stack, leading to stack confusion.

[ ... ]
> +	daddr.a4 = ip_hdr(skb)->daddr;
> +	x = xfrm_input_state_lookup(dev_net(rx_dev), skb->mark, &daddr,
> +				    info.spi, IPPROTO_ESP, AF_INET);
> +	if (!x)
> +		return false;
> +
> +	if (x->xso.dir != XFRM_DEV_OFFLOAD_IN)
> +		goto put_state;
> +	if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +		goto put_state;
> +	if (x->xso.dev != rx_dev)
> +		goto put_state;

[Severity: High]
Will this strict check reject offloaded ESP packets for DSA user ports?

For DSA SAs, the offload device is the DSA user port. However, inbound ESP
packets are intercepted at the physical conduit device before DSA unpacks
the tags, meaning the received device is the conduit. The packet is ignored
for hardware decrypt offload because the conduit does not match the user
port.

Furthermore, intercepting right after eth_type_trans() means the data
points to the DSA tag rather than the IP header, which would cause IPv4
parsing to fail.

[ ... ]
> +static bool airoha_soe_complete_out(struct sk_buff *skb,
> +				    struct airoha_soe_ctx *ctx)
> +{
> +	struct dst_entry *dst = ctx->dst;
> +	struct net *net;
> +	int err;
> +
> +	ctx->dst = NULL;
[ ... ]
> +	skb->protocol = htons(ETH_P_IP);
> +	skb_dst_drop(skb);
> +	skb_dst_set(skb, dst);
> +	skb->ignore_df = 1;
> +	net = dev_net(dst->dev);
> +	kfree(ctx);
> +	dst_output(net, NULL, skb);

[Severity: High]
Does this reinjection drop critical SKB metadata?

It looks like only the routing entry is restored before calling
dst_output(), failing to carry over metadata like the packet mark,
priority, socket association, and VLAN TCI from the original SKB.

Could this cause the transmitted outer IPsec packet to lose its QoS
classifications and socket accounting contexts?

[ ... ]
> +static bool airoha_soe_complete_in(struct sk_buff *skb,
> +				   struct airoha_soe_ctx *ctx)
> +{
[ ... ]
> +	skb->dev = rx_dev;
> +	skb->mark = ctx->rx.mark;
> +	skb->ip_summed = CHECKSUM_NONE;
> +	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);
> +	skb->pkt_type = PACKET_HOST;
> +	skb->encapsulation = 0;
> +	skb_dst_drop(skb);
[ ... ]
> +	kfree(ctx);
> +	netif_rx(skb);

[Severity: High]
Will injecting a MAC-less L3 packet via netif_rx() cause issues in the
network stack?

The function strips the dummy L2 header but sets the MAC header to point
directly to the IP header. Since the network stack expects packets arriving
on physical Ethernet devices to possess a valid MAC header, reading the IP
header as an Ethernet header breaks protocol semantics.

Could this poison the bridge FDB with garbage MAC addresses derived from
IP fields?

[ ... ]
> +static int airoha_soe_xfrm_state_add(struct net_device *dev,
> +				     struct xfrm_state *x,
> +				     struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	state->dev = gdm_dev;
> +	state->soe = soe;

[Severity: Critical]
Could this lead to a Use-After-Free of the conduit network device private
data?

When an XFRM state is added to a DSA user port, the private data is
assigned a pointer to the conduit device without taking a reference on the
conduit netdev via dev_hold().

If the conduit device is unregistered, the DSA user ports are destroyed,
triggering an asynchronous XFRM state garbage collection. The background GC
could then dereference the dangling state pointer in
airoha_soe_xfrm_state_free() if the conduit device was fully freed.

[ ... ]
> +static void airoha_soe_dsa_proxy_enable(struct net_device *dev)
> +{
> +	struct net_device *conduit;
> +
> +	conduit = airoha_soe_dsa_conduit_get(dev);
> +	if (!conduit)
> +		return;
> +
> +	if (dev->xfrmdev_ops && dev->xfrmdev_ops != &airoha_soe_dsa_xfrmdev_ops)
> +		goto out;

[Severity: Medium]
Does this condition fail to exit early if the ops are already set?

Since airoha_soe_netdev_event() calls airoha_soe_dsa_proxy_scan(true) on
every NETDEV_REGISTER event, the lack of an early return when the ops
pointer is already equal to airoha_soe_dsa_xfrmdev_ops causes it to proceed
and call netdev_update_features() redundantly.

This means adding any unrelated interface to the system triggers feature
recalculations on all existing DSA ports.

[ ... ]
> +static void airoha_soe_dsa_proxy_scan(bool enable)
> +{
> +	struct net_device *dev;
> +
> +	for_each_netdev(&init_net, dev) {
> +		if (enable)
> +			airoha_soe_dsa_proxy_enable(dev);
> +		else
> +			airoha_soe_dsa_proxy_clear(dev);
> +	}
> +}

[Severity: High]
Is it intentional to restrict this loop to init_net?

If a DSA user interface is moved to a different network namespace, it
appears this loop will permanently skip it. During module unload,
airoha_soe_deinit() calls this scan to clear the hook pointers, but failing
to reach interfaces outside init_net leaves dangling pointers to
airoha_soe_dsa_xfrmdev_ops.

Could this result in a Use-After-Free and kernel panic upon a subsequent
network namespace cleanup or XFRM operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614040032.1567994-1-hurryman2212@gmail.com?part=7