[U-Boot] [PATCH] phy: Fix u-boot coruption when fixed-phy is used

[U-Boot] [PATCH] phy: Fix u-boot coruption when fixed-phy is used

[Michal Simek]

When fixed-link phy is used subnode offset is used as phy address. This
number is bigger then space allocated for bus structure (allocated via
mdio_alloc).
bus->phymap[] array has PHY_MAX_ADDR size (32).
That's why writing bus->phymap[addr] where addr is < 0 or > PHY_MAX_ADDR
is causing write to memory which can caused full U-Boot crash.

The patch is checking if address is in correct range.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

search_for_existing_phy() is using this array but there is if check
already that's why it shouldn't be a big deal.

---
 drivers/net/phy/phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e837eb7688cc..cda4caa8034d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -656,7 +656,8 @@ static struct phy_device *phy_device_create(struct mii_dev *bus, int addr,
 
 	phy_probe(dev);
 
-	bus->phymap[addr] = dev;
+	if (addr >= 0 && addr < PHY_MAX_ADDR)
+		bus->phymap[addr] = dev;
 
 	return dev;
 }
-- 
1.9.1

[U-Boot] [PATCH] phy: Fix u-boot coruption when fixed-phy is used

[Stefan Roese]

On 19.12.18 16:57, Michal Simek wrote:
> When fixed-link phy is used subnode offset is used as phy address. This
> number is bigger then space allocated for bus structure (allocated via
> mdio_alloc).
> bus->phymap[] array has PHY_MAX_ADDR size (32).
> That's why writing bus->phymap[addr] where addr is < 0 or > PHY_MAX_ADDR
> is causing write to memory which can caused full U-Boot crash.
> 
> The patch is checking if address is in correct range.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> search_for_existing_phy() is using this array but there is if check
> already that's why it shouldn't be a big deal.
> 
> ---
>   drivers/net/phy/phy.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e837eb7688cc..cda4caa8034d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -656,7 +656,8 @@ static struct phy_device *phy_device_create(struct mii_dev *bus, int addr,
>   
>   	phy_probe(dev);
>   
> -	bus->phymap[addr] = dev;
> +	if (addr >= 0 && addr < PHY_MAX_ADDR)
> +		bus->phymap[addr] = dev;

How about adding some error / warning print in the else path here?

Thanks,
Stefan

[U-Boot] [PATCH] phy: Fix u-boot coruption when fixed-phy is used

[Michal Simek]

On 20. 12. 18 14:13, Stefan Roese wrote:
> On 19.12.18 16:57, Michal Simek wrote:
>> When fixed-link phy is used subnode offset is used as phy address. This
>> number is bigger then space allocated for bus structure (allocated via
>> mdio_alloc).
>> bus->phymap[] array has PHY_MAX_ADDR size (32).
>> That's why writing bus->phymap[addr] where addr is < 0 or > PHY_MAX_ADDR
>> is causing write to memory which can caused full U-Boot crash.
>>
>> The patch is checking if address is in correct range.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> search_for_existing_phy() is using this array but there is if check
>> already that's why it shouldn't be a big deal.
>>
>> ---
>>   drivers/net/phy/phy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index e837eb7688cc..cda4caa8034d 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -656,7 +656,8 @@ static struct phy_device *phy_device_create(struct
>> mii_dev *bus, int addr,
>>         phy_probe(dev);
>>   -    bus->phymap[addr] = dev;
>> +    if (addr >= 0 && addr < PHY_MAX_ADDR)
>> +        bus->phymap[addr] = dev;
> 
> How about adding some error / warning print in the else path here?

We can add it but I am not quite sure how that error should look like in
this case. Because addr is filled by node offset got from
sn = fdt_first_subnode(gd->fdt_blob, dev_of_offset(dev));

It means this error/warning will be shown all the time which is also weird.

The solution could be to save link to phy DT node in phy_device
structure and then read it on driver not like this
int ofnode = phydev->addr;
which is done in fixed.c

But anyway still that above fix make sense to make sure that we are not
overwriting stuff.

Thanks,
Michal

[U-Boot] phy: Fix u-boot coruption when fixed-phy is used

[Tom Rini]

On Wed, Dec 19, 2018 at 04:57:38PM +0100, Michal Simek wrote:

> When fixed-link phy is used subnode offset is used as phy address. This
> number is bigger then space allocated for bus structure (allocated via
> mdio_alloc).
> bus->phymap[] array has PHY_MAX_ADDR size (32).
> That's why writing bus->phymap[addr] where addr is < 0 or > PHY_MAX_ADDR
> is causing write to memory which can caused full U-Boot crash.
> 
> The patch is checking if address is in correct range.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181227/36b3ea7d/attachment.sig>