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

Mugunthan V N
2013-09-05T08:27:55Z

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@denx.de>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: Tom Rini <trini@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