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

[Heiko Schocher]

Hello Mugunthan,

Am 05.09.2013 10:27, schrieb 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>
>>
>> ---
[...]
>> 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.

No, cpsw_update_link() is called for example directly from cpsw_init()
cpsw_set_slave_mac() from cpsw_slave_init()

>> +	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.

Yes, you are right. Checked it on the dxr2 board. We do not need
the "priv check" ... Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany