[PATCH net] macsec: check offload ops before inserting TX tag

[PATCH net] macsec: check offload ops before inserting TX tag

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

macsec_insert_tx_tag() takes the ops pointer returned by
macsec_get_ops() and dereferences it straight away:

	ops = macsec_get_ops(macsec, &ctx);
	skb_final_len = skb->len - ETH_HLEN + ops->needed_headroom +
		ops->needed_tailroom;

macsec_get_ops() returns NULL whenever the offload is no longer live:
for MACSEC_OFFLOAD_PHY, macsec_check_offload() requires
real_dev->phydev to still be attached.

phy_detach() clears real_dev->phydev, e.g. when the lower device is
brought down and its driver calls phy_disconnect() from ndo_stop(),
when the PHY driver is unbound, or when an SFP module is removed under
phylink. A frame sent after that point reaches macsec_insert_tx_tag()
with ops == NULL and oopses.

Check the pointer, as every other macsec_get_ops() caller already
does, and drop the frame through the existing cleanup path.

Fixes: a73d8779d61a ("net: macsec: introduce mdo_insert_tx_tag")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/net/macsec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index ee0e2eb7dbc6..86c8009dcfa9 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3436,6 +3436,11 @@ static struct sk_buff *macsec_insert_tx_tag(struct sk_buff *skb,
 	int err;
 
 	ops = macsec_get_ops(macsec, &ctx);
+	if (unlikely(!ops)) {
+		err = -EOPNOTSUPP;
+		goto cleanup;
+	}
+
 	skb_final_len = skb->len - ETH_HLEN + ops->needed_headroom +
 		ops->needed_tailroom;
 	if (unlikely(skb_final_len > macsec->real_dev->mtu)) {

---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260805-macsec-fixes-d280baf70552

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

Re: [PATCH net] macsec: check offload ops before inserting TX tag

[Antoine Tenart]

On Wed, Aug 05, 2026 at 12:52:10PM +0800, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> macsec_insert_tx_tag() takes the ops pointer returned by
> macsec_get_ops() and dereferences it straight away:
> 
> 	ops = macsec_get_ops(macsec, &ctx);
> 	skb_final_len = skb->len - ETH_HLEN + ops->needed_headroom +
> 		ops->needed_tailroom;

phydev is read again later in macsec_insert_tx_tag, that probably needs
a check too (I don't think it is used in practice).

> macsec_get_ops() returns NULL whenever the offload is no longer live:
> for MACSEC_OFFLOAD_PHY, macsec_check_offload() requires
> real_dev->phydev to still be attached.

From what I can tell the same issue could happen within
macsec_check_offload and __macsec_get_ops too (and so macsec_get_ops).

Also looks like handle_not_macsec has a similar construction, does it
need a check?

> phy_detach() clears real_dev->phydev, e.g. when the lower device is
> brought down and its driver calls phy_disconnect() from ndo_stop(),
> when the PHY driver is unbound, or when an SFP module is removed under
> phylink. A frame sent after that point reaches macsec_insert_tx_tag()
> with ops == NULL and oopses.
> 
> Check the pointer, as every other macsec_get_ops() caller already
> does, and drop the frame through the existing cleanup path.

Looking at phy_detach, could the PHY device also get freed? If so things
could get wild, but that is probably out of scope here.

> Fixes: a73d8779d61a ("net: macsec: introduce mdo_insert_tx_tag")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
>  drivers/net/macsec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index ee0e2eb7dbc6..86c8009dcfa9 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -3436,6 +3436,11 @@ static struct sk_buff *macsec_insert_tx_tag(struct sk_buff *skb,
>  	int err;
>  
>  	ops = macsec_get_ops(macsec, &ctx);
> +	if (unlikely(!ops)) {
> +		err = -EOPNOTSUPP;
> +		goto cleanup;
> +	}
> +
>  	skb_final_len = skb->len - ETH_HLEN + ops->needed_headroom +
>  		ops->needed_tailroom;
>  	if (unlikely(skb_final_len > macsec->real_dev->mtu)) {
> 
> ---
> base-commit: 075b74841bd0065a3bda3440873c747938e69b68
> change-id: 20260805-macsec-fixes-d280baf70552
> 
> Best regards,
> -- 
> Junrui Luo <moonafterrain@outlook.com>
> 
> 

Re: [PATCH net] macsec: check offload ops before inserting TX tag

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
macsec: check offload ops before inserting TX tag

This commit fixes a NULL pointer dereference in macsec_insert_tx_tag().
When phy_detach() clears the lower device's phydev, macsec_get_ops()
returns NULL. This patch adds a check to safely drop the frame instead
of dereferencing the NULL ops pointer.

> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index ee0e2eb7dbc61..86c8009dcfa98 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -3436,6 +3436,11 @@ static struct sk_buff *macsec_insert_tx_tag(struct sk_buff *skb,
>  	int err;
>  
>  	ops = macsec_get_ops(macsec, &ctx);
> +	if (unlikely(!ops)) {
> +		err = -EOPNOTSUPP;
> +		goto cleanup;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does macsec_insert_tx_tag()
still risk a NULL pointer dereference or Use-After-Free?

Further down in macsec_insert_tx_tag(), we re-read the phydev directly from
the real_dev without any synchronization:

drivers/net/macsec.c:macsec_insert_tx_tag() {
    ...
    phydev = macsec->real_dev->phydev;
    ...
    err = ops->mdo_insert_tx_tag(phydev, skb);
    ...
}

If phy_detach() runs concurrently and clears real_dev->phydev, couldn't this
pass a NULL or freed pointer to mdo_insert_tx_tag()?

Additionally, macsec_check_offload() and __macsec_get_ops() appear to perform
multiple unguarded reads of macsec->real_dev->phydev. Since
phy_device_release() frees the device directly rather than via RCU, could these
lockless dereferences lead to memory corruption, as the datapath's
rcu_read_lock_bh() doesn't protect the phy_device?

[Severity: High]
This is a pre-existing issue, but doesn't the RX datapath have a similar
NULL pointer dereference?

Looking at handle_not_macsec(), if phy_detach() clears real_dev->phydev
concurrently, macsec_get_ops() can return NULL.

drivers/net/macsec.c:handle_not_macsec() {
    ...
    ops = macsec_get_ops(macsec, NULL);

    if (ops->rx_uses_md_dst && !is_macsec_md_dst)
        continue;
    ...
}

Since the returned ops pointer is immediately dereferenced without a check,
will this cause a crash when the PHY is detached?

>  	skb_final_len = skb->len - ETH_HLEN + ops->needed_headroom +
>  		ops->needed_tailroom;
>  	if (unlikely(skb_final_len > macsec->real_dev->mtu)) {
-- 
pw-bot: cr