[PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Chuanhong Guo]

The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
and a phy-handle isn't always available.
Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
connected to switch port 5 and setup mt7530 according to phy address
of 2nd gmac node, causing null pointer dereferencing when phy-handle
isn't defined in dts.
This commit fix this setup code by checking return value of
of_parse_phandle before using it.

Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
Cc: stable@vger.kernel.org
---

mt7530 is available as a standalone chip and we should not make it
tightly coupled with a specific type of ethernet dt binding in the
first place.
A proper fix is to replace this port detection logic with a dt
property under mt7530 node, but that's too much for linux-stable.

 drivers/net/dsa/mt7530.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6e91fe2f4b9a..1d53a4ebcd5a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1414,6 +1414,9 @@ mt7530_setup(struct dsa_switch *ds)
 				continue;
 
 			phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
+			if (!phy_node)
+				continue;
+
 			if (phy_node->parent == priv->dev->of_node->parent) {
 				ret = of_get_phy_mode(mac_np, &interface);
 				if (ret && ret != -ENODEV)
-- 
2.25.1

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Vivien Didelot]

On Fri,  3 Apr 2020 19:28:24 +0800, Chuanhong Guo <gch981213@gmail.com> wrote:
> The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> and a phy-handle isn't always available.
> Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> connected to switch port 5 and setup mt7530 according to phy address
> of 2nd gmac node, causing null pointer dereferencing when phy-handle
> isn't defined in dts.
> This commit fix this setup code by checking return value of
> of_parse_phandle before using it.
> 
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Florian Fainelli]

On 4/3/2020 4:28 AM, Chuanhong Guo wrote:
> The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> and a phy-handle isn't always available.
> Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> connected to switch port 5 and setup mt7530 according to phy address
> of 2nd gmac node, causing null pointer dereferencing when phy-handle
> isn't defined in dts.
> This commit fix this setup code by checking return value of
> of_parse_phandle before using it.
> 
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
> 
> mt7530 is available as a standalone chip and we should not make it
> tightly coupled with a specific type of ethernet dt binding in the
> first place.
> A proper fix is to replace this port detection logic with a dt
> property under mt7530 node, but that's too much for linux-stable.

Agree, one would also wonder why the driver needs to parse parts of the
Device Tree which should be done by the core DSA layer.
-- 
Florian

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[René van Dorst]

Quoting Chuanhong Guo <gch981213@gmail.com>:

Hi Chuanhong,

> The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> and a phy-handle isn't always available.
> Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> connected to switch port 5 and setup mt7530 according to phy address
> of 2nd gmac node, causing null pointer dereferencing when phy-handle
> isn't defined in dts.

MT7530 tries to detect if 2nd GMAC is using a phy with phy-address 0 or 4.
If so, switch port 5 needs to be setup so that PHY 0 or 4 is available
via port 5 of the switch. Any MAC can talk to PHY 0/4 directly via port 5.
This is also explained in the kernel docs mt7530.txt.

May be there are better way to detect that any node is using phy 0/4 of
the switch.

Funny that I never tested this case that 2nd gmac node exits and is disabled
without using port 5.

Thanks for the fix.

Tested-by: René van Dorst <opensource@vdorst.com>

Greats,

René

> This commit fix this setup code by checking return value of
> of_parse_phandle before using it.
>
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>
> mt7530 is available as a standalone chip and we should not make it
> tightly coupled with a specific type of ethernet dt binding in the
> first place.
> A proper fix is to replace this port detection logic with a dt
> property under mt7530 node, but that's too much for linux-stable.
>
>  drivers/net/dsa/mt7530.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 6e91fe2f4b9a..1d53a4ebcd5a 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1414,6 +1414,9 @@ mt7530_setup(struct dsa_switch *ds)
>  				continue;
>
>  			phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> +			if (!phy_node)
> +				continue;
> +
>  			if (phy_node->parent == priv->dev->of_node->parent) {
>  				ret = of_get_phy_mode(mac_np, &interface);
>  				if (ret && ret != -ENODEV)
> --
> 2.25.1

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[David Miller]

From: Chuanhong Guo <gch981213@gmail.com>
Date: Fri,  3 Apr 2020 19:28:24 +0800

> The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> and a phy-handle isn't always available.
> Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> connected to switch port 5 and setup mt7530 according to phy address
> of 2nd gmac node, causing null pointer dereferencing when phy-handle
> isn't defined in dts.
> This commit fix this setup code by checking return value of
> of_parse_phandle before using it.
> 
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> Cc: stable@vger.kernel.org

Please do not CC: stable for networking changes, as per:

	Documentation/networking/netdev-FAQ.rstq

Applied and queued up for -stable, thank you.

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Chuanhong Guo]

Hi!

On Sat, Apr 4, 2020 at 2:09 AM René van Dorst <opensource@vdorst.com> wrote:
>
> Quoting Chuanhong Guo <gch981213@gmail.com>:
>
> Hi Chuanhong,
>
> > The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> > and a phy-handle isn't always available.
> > Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> > connected to switch port 5 and setup mt7530 according to phy address
> > of 2nd gmac node, causing null pointer dereferencing when phy-handle
> > isn't defined in dts.
>
> MT7530 tries to detect if 2nd GMAC is using a phy with phy-address 0 or 4.

What if the 2nd GMAC connects to an external PHY on address 0 on a
different mdio-bus? This is already happening on mt7629 where the
integrated PHY connected to gmac2 is on address 0 and gmac1
connects to external mt753x switch.
On mt7621, the RGMII2 is always wired to switch mac5 as well as
external pins, and one should disable switch mac5 in this case or
there's pin conflict.

> If so, switch port 5 needs to be setup so that PHY 0 or 4 is available
> via port 5 of the switch. Any MAC can talk to PHY 0/4 directly via port 5.
> This is also explained in the kernel docs mt7530.txt.
>
> May be there are better way to detect that any node is using phy 0/4 of
> the switch.

It should never be detected. This piece of code is to configure how
RGMII2 on mt7530 is wired: PHY0/PHY4/switch MAC5/off. And it
should be implemented as a configurable dt property. We should
not make assumption that 2 RGMIIs always connected to the two
macs on mtk_eth_soc and we should never parse parent eth dts
in DSA driver.

-- 
Regards,
Chuanhong Guo

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Chuanhong Guo]

Hi!

On Sat, Apr 4, 2020 at 7:11 AM David Miller <davem@davemloft.net> wrote:
> > Cc: stable@vger.kernel.org
>
> Please do not CC: stable for networking changes, as per:
>
>         Documentation/networking/netdev-FAQ.rstq

Oh! I'm not aware of this doc. Thanks for pointing it out.

-- 
Regards,
Chuanhong Guo

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Andrew Lunn]

On Sat, Apr 04, 2020 at 11:19:10AM +0800, Chuanhong Guo wrote:
> Hi!
> 
> On Sat, Apr 4, 2020 at 2:09 AM René van Dorst <opensource@vdorst.com> wrote:
> >
> > Quoting Chuanhong Guo <gch981213@gmail.com>:
> >
> > Hi Chuanhong,
> >
> > > The 2nd gmac of mediatek soc ethernet may not be connected to a PHY
> > > and a phy-handle isn't always available.
> > > Unfortunately, mt7530 dsa driver assumes that the 2nd gmac is always
> > > connected to switch port 5 and setup mt7530 according to phy address
> > > of 2nd gmac node, causing null pointer dereferencing when phy-handle
> > > isn't defined in dts.
> >
> > MT7530 tries to detect if 2nd GMAC is using a phy with phy-address 0 or 4.
> 
> What if the 2nd GMAC connects to an external PHY on address 0 on a
> different mdio-bus?

In general, you using a phy-handle to cover such a situation. If there
is a phy-handle, just use it.

   Andrew

Re: [PATCH] net: dsa: mt7530: fix null pointer dereferencing in port5 setup

[Chuanhong Guo]

Hi!

On Sat, Apr 4, 2020 at 11:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > MT7530 tries to detect if 2nd GMAC is using a phy with phy-address 0 or 4.
> >
> > What if the 2nd GMAC connects to an external PHY on address 0 on a
> > different mdio-bus?
>
> In general, you using a phy-handle to cover such a situation. If there
> is a phy-handle, just use it.

If it's determining where switch mac5 is wired, a phy-handle is fine.
Here we are determining where exposed rgmii2 pins are wired.
It can be wired to switch mac5 or skip the switch mac completely
and connected to phy0/phy4.
Current driver is determining rgmii2 wiring on mt7530 using phy-handle
on *another unrelated ethernet node* which doesn't sound right.

-- 
Regards,
Chuanhong Guo