[U-Boot] [PATCH v2] net, phy, cpsw: fix NULL pointer deference

[Mugunthan V N]

On Thursday 05 September 2013 11:35 AM, Heiko Schocher wrote:
> if phy_connect() did not find a phy, phydev is NULL and
> following code in cpsw_phy_init() crashes. Fix this.
>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> Cc: Joe Hershberger <joe.hershberger at gmail.com>
> Cc: Mugunthan V N <mugunthanvnm at ti.com>
> Cc: Tom Rini <trini at ti.com>
>
> ---
> - changes for v2:
>   - change commit message as it is a NULL pointer deference error
>     not an unaligned access problem, as Tom Rini mentioned
>   - fix more places in the driver with NULL pointer problem
>
> Found on the dxr2 board with no phy connected to the board,
> U-Boot crashes with:
>
> U-Boot 2013.07-12701-gea98378-dirty (Sep 04 2013 - 06:58:16)
>
> I2C:   ready
> DRAM:  128 MiB
> Enable d-cache
> FactorySet is not right in eeprom.
> NAND:  256 MiB
> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
> 8-bit BCH HW ECC selected
> Net:   Could not get PHY for cpsw: addr 0
> data abort
>
>     MAYBE you should read doc/README.arm-unaligned-accesses
>
> pc : [<87f80574>]          lr : [<87f80fcc>]
> sp : 86f5aee0  ip : 00000034     fp : 80100020
> r10: 00000014  r9 : 07e5d000     r8 : 86f5af30
> r7 : 86f5f750  r6 : 86f5f804     r5 : 86f5f708  r4 : 86f5f750
> r3 : 00000000  r2 : 00000000     r1 : 87fa4d08  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32
> Resetting CPU ...
> ---
>  drivers/net/cpsw.c | 26 +++++++++++++++++++++++++-
>  1 Datei geändert, 25 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>
> diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c
> index 9bab71a..186665b 100644
> --- a/drivers/net/cpsw.c
> +++ b/drivers/net/cpsw.c
> @@ -561,6 +561,9 @@ static inline void setbit_and_wait_for_clear32(void *addr)
>  static void cpsw_set_slave_mac(struct cpsw_slave *slave,
>  			       struct cpsw_priv *priv)
>  {
> +	if (!priv)
> +		return;
> +
>  	__raw_writel(mac_hi(priv->dev->enetaddr), &slave->regs->sa_hi);
>  	__raw_writel(mac_lo(priv->dev->enetaddr), &slave->regs->sa_lo);
>  }
> @@ -568,9 +571,17 @@ static void cpsw_set_slave_mac(struct cpsw_slave *slave,
>  static void cpsw_slave_update_link(struct cpsw_slave *slave,
>  				   struct cpsw_priv *priv, int *link)
>  {
> -	struct phy_device *phy = priv->phydev;
> +	struct phy_device *phy;
>  	u32 mac_control = 0;
>  
> +	if (!priv)
> +		return;
> +
> +	phy = priv->phydev;
> +
> +	if (!phy)
> +		return;
> +
>  	phy_startup(phy);
>  	*link = phy->link;
>  
> @@ -604,8 +615,12 @@ static int cpsw_update_link(struct cpsw_priv *priv)
>  	int link = 0;
>  	struct cpsw_slave *slave;
>  
> +	if (!priv)
> +		return -1;
> +
>  	for_each_slave(slave, priv)
>  		cpsw_slave_update_link(slave, priv, &link);
> +
>  	priv->mdio_link = readl(&mdio_regs->link);
>  	return link;
>  }
> @@ -614,6 +629,9 @@ static int cpsw_check_link(struct cpsw_priv *priv)
>  {
>  	u32 link = 0;

All the above *priv* check can be remove as priv is already validated in
this function and all the above functions are called only from this
function.

>  
> +	if (!priv)
> +		return -1;
> +
>  	link = __raw_readl(&mdio_regs->link) & priv->phy_mask;
>  	if ((link) && (link == priv->mdio_link))
>  		return 1;
> @@ -623,6 +641,9 @@ static int cpsw_check_link(struct cpsw_priv *priv)
>  
>  static inline u32  cpsw_get_slave_port(struct cpsw_priv *priv, u32 slave_num)
>  {
> +	if (!priv)
> +		return -1;
> +
>  	if (priv->host_port == 0)
>  		return slave_num + 1;
>  	else
> @@ -947,6 +968,9 @@ static int cpsw_phy_init(struct eth_device *dev, struct cpsw_slave *slave)
>  			dev,
>  			slave->data->phy_if);
>  
> +	if (!phydev)
> +		return -1;
> +
>  	phydev->supported &= supported;
>  	phydev->advertising = phydev->supported;
>  
I don't think you need to validate *priv* as it is already validated in
driver register (cpsw_register()). In any case priv is NULL, then there
is some corruption in dev structure as without priv, dev should not
exist and stack is not supposed to call any cpsw apis.

Regards
Mugunthan V N