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

[Jakub Kicinski <kuba@kernel.org>]

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