Re: [PATCH net-next v2 3/6] net: ftgmac100: Enable AST2700 RMII support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: ftgmac100: NULL pointer dereference in ftgmac100_start_hw()
  on missing PHY

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  copies it.

--

net: ftgmac100: Enable AST2700 RMII support

This commit adds a FTGMAC100_QUIRK_RMII_ENABLE match-data quirk for
AST2700. ftgmac100_start_hw() uses the quirk to set the new
FTGMAC100_MACCR_RMII_ENABLE bit when the attached PHY uses RMII.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 67b1fa464a421..b835472da3604 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -355,6 +359,7 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
>  static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  {
>  	u32 maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
> +	struct phy_device *phydev = priv->netdev->phydev;
>  
>  	/* Keep the original GMAC and FAST bits */
>  	maccr &= (FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_GIGA_MODE);
> @@ -383,6 +388,11 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  	if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
>  		maccr |= FTGMAC100_MACCR_RM_VLAN;
>  
> +	if ((priv->quirks & FTGMAC100_QUIRK_RMII_ENABLE) &&
> +	    phydev->interface == PHY_INTERFACE_MODE_RMII) {

[Severity: High]
Can phydev be NULL here?

ftgmac100_probe_dt() returns 0 without attaching a PHY when the node
has an mdio child but no phy-handle, fixed-link or use-ncsi property.
of_phy_get_and_connect() is skipped in that case. The
ftgmac100_mii_probe() fallback is skipped too, because the mdio child
exists. Probe then registers the netdev with netdev->phydev still NULL.

The rest of the driver treats a missing PHY as a valid state. For
example, ftgmac100_open() and ftgmac100_stop() test netdev->phydev
before using it. This check reads phydev->interface whenever the quirk
is set, and ftgmac100_match_data_ast2700 always sets it. On such a
board, bringing the interface up would oops via ftgmac100_init_all()
-> ftgmac100_start_hw(). The MAC restart in ftgmac100_poll() and
ftgmac100_set_rx_mode() reach the same code.

ftgmac100_reset_and_config_mac() already does the same unchecked
priv->netdev->phydev->interface read on Aspeed parts. It runs before
ftgmac100_start_hw() on open, so that site may oops first today. This
is still a second dereference that depends only on the quirk bit. The
quirk is meant to be reused by later controllers, which may not set
is_aspeed.

The later patch requiring phy-mode on AST2700 only changes
ftgmac100_mii_probe(). This path never reaches that function, so
nothing in the series closes the gap.

Could phydev be checked for NULL here and in
ftgmac100_reset_and_config_mac()? Or should probe fail on AST2700 when
no PHY ends up attached?

[Severity: High]
This is a pre-existing issue, but this new check repeats it. Can phydev be
NULL here?

ftgmac100_reset_and_config_mac() already dereferences phydev the same way,
without a check, for every Aspeed MAC. That code came from commit
af350ee72e9dda:

    if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII) {

The faraday,ftgmac100.yaml schema seems to allow a node that has an mdio
child but no phy-handle, fixed-link or use-ncsi. For such a node,
ftgmac100_probe_dt() skips of_phy_get_and_connect(). Because the mdio child
is present, it also skips the ftgmac100_mii_probe() fallback:

    if (!ftgmac100_has_child_node(np, "mdio")) {
        ...
        err = ftgmac100_mii_probe(netdev);
        ...
    }

    return 0;

Probe then registers the netdev with no PHY attached.

Other parts of the driver accept a NULL phydev, for example
ftgmac100_open():

    if (netdev->phydev) {
        /* If we have a PHY, start polling */
        phy_start(netdev->phydev);

ftgmac100_open()->ftgmac100_reset_and_config_mac() dereferences phydev
before that check, though, so the oops happens there on ndo_open.
ftgmac100_reset() takes the same path.

On AST2700, every caller of ftgmac100_start_hw() runs after
ftgmac100_reset_and_config_mac(). That covers ftgmac100_init_all(),
ftgmac100_poll() via need_mac_restart, and ftgmac100_set_rx_mode(). So the
new site does not add a crash on its own.

It does add a second unchecked read that depends only on the quirk bit, and
AST2700 always sets that bit. A future controller that sets the quirk
without is_aspeed would crash here.

Later patches in the series leave both sites unchecked. "net: ftgmac100:
Require phy-mode for AST2700" only changes ftgmac100_mii_probe(), and this
path never reaches that function.

Could phydev be checked for NULL at both sites? Another option would be to
fail probe on Aspeed parts when no PHY gets attached.

> +		maccr |= FTGMAC100_MACCR_RMII_ENABLE;
> +	}
> +
>  	/* Hit the HW */
>  	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-support_ast2700-v2-0-36de51fb8066%40aspeedtech.com