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

On Wed, Aug 05, 2026 at 12:52:10PM +0800, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <[email protected]>
> 
> 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 <[email protected]>
> Assisted-by: Claude:claude-opus-5
> Cc: [email protected]
> Signed-off-by: Junrui Luo <[email protected]>
> ---
>  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 <[email protected]>
> 
>