| Message ID | 20180903084853.18092-4-quentin.schulz@bootlin.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2,1/7] net: phy: mscc: factorize code for LEDs mode | expand |
On Mon, Sep 03, 2018 at 10:48:50AM +0200, Quentin Schulz wrote: > In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown' > is an u8, even though it's read as an u8 in the driver. Hi Quentin of_property_read_u8() will perform bounds checking. Is that important here? Thanks Andrew
Hi Andrew, On Mon, Sep 03, 2018 at 03:27:56PM +0200, Andrew Lunn wrote: > On Mon, Sep 03, 2018 at 10:48:50AM +0200, Quentin Schulz wrote: > > In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown' > > is an u8, even though it's read as an u8 in the driver. > > Hi Quentin > > of_property_read_u8() will perform bounds checking. Is that important > here? > Just to be sure, we're talking here about making sure the value stored in the DT is not bigger than the specified value (here an u8)? If so, that isn't the reason why I'm suggesting those two patches. Without /bits 8/ in the DT property, whatever were the values I put in the property, I'd always get a 0. So I need to fix it either in the DT (but Rob does not really like it) or in the driver. Hope that makes sense. Quentin
> Just to be sure, we're talking here about making sure the value stored > in the DT is not bigger than the specified value (here an u8)? If so, > that isn't the reason why I'm suggesting those two patches. > > Without /bits 8/ in the DT property, whatever were the values I put in > the property, I'd always get a 0. So I need to fix it either in the DT > (but Rob does not really like it) or in the driver. Hi Quentin Ah, you are fixing endian issues. That was not clear to me from the commit message. I don't know enough about how DT stores values in the blob. Is there type info? Can the DT core tell if a value in the blob is a u8 or a u32? It would be nice if it warned about reading a u8 from a u32 blob. Anyway, this change still removes some bounds checking. Are they important? Do they need to be added back? Thanks Andrew
Hi Andrew, On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote: > > Just to be sure, we're talking here about making sure the value stored > > in the DT is not bigger than the specified value (here an u8)? If so, > > that isn't the reason why I'm suggesting those two patches. > > > > Without /bits 8/ in the DT property, whatever were the values I put in > > the property, I'd always get a 0. So I need to fix it either in the DT > > (but Rob does not really like it) or in the driver. > > Hi Quentin > > Ah, you are fixing endian issues. That was not clear to me from the > commit message. > > I don't know enough about how DT stores values in the blob. Is there > type info? Can the DT core tell if a value in the blob is a u8 or a > u32? It would be nice if it warned about reading a u8 from a u32 > blob. > From my quick research, the lower bound checking is performed by of_property_read_u* functions but not the higher bound checking (the internal function of_find_property_value_of_size allows higher bound checking but it seems it's never used by those functions (see 0 in sz_max of of_property_read_variable_u*_array)). sz_max is used by of_property_read_variable_u*_array to copy at a maximum of sz_max values in the output buffer. If sz_max is 0, it takes sz_min so it's an array of definite size. So since sz_max is 0 for all calls to of_property_read_variable_u*_array by of_property_read_u*_array, we basically know we'll get a buffer of sz_min values but we don't actually make use of the higher bound checking of of_find_property_value_of_size. We could enforce this higher bound check by, instead of setting sz_max to 0, setting sz_max to sz_min in calls to of_property_read_u*_array. But I guess there is a reason for sz_max being 0. Rob, Richard (commit signer of this code) do you know why? Could you explain? > Anyway, this change still removes some bounds checking. Are they > important? Do they need to be added back? > The edge-slowdown and the vddmac values are compared against a const array so we´re fine with those ones. For the led-X-mode, I added a constant for supported modes that gets checked when retrieving the DT property. So we´re fine here too. Quentin
On 04/09/18 08:26, Quentin Schulz wrote: > Hi Andrew, > > On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote: >>> Just to be sure, we're talking here about making sure the value stored >>> in the DT is not bigger than the specified value (here an u8)? If so, >>> that isn't the reason why I'm suggesting those two patches. >>> >>> Without /bits 8/ in the DT property, whatever were the values I put in >>> the property, I'd always get a 0. So I need to fix it either in the DT >>> (but Rob does not really like it) or in the driver. >> >> Hi Quentin >> >> Ah, you are fixing endian issues. That was not clear to me from the >> commit message. >> >> I don't know enough about how DT stores values in the blob. Is there >> type info? Can the DT core tell if a value in the blob is a u8 or a >> u32? It would be nice if it warned about reading a u8 from a u32 >> blob. >> > > From my quick research, the lower bound checking is performed by > of_property_read_u* functions but not the higher bound checking (the > internal function of_find_property_value_of_size allows higher bound > checking but it seems it's never used by those functions (see 0 in > sz_max of of_property_read_variable_u*_array)). > > sz_max is used by of_property_read_variable_u*_array to copy at a > maximum of sz_max values in the output buffer. If sz_max is 0, it takes > sz_min so it's an array of definite size. > So since sz_max is 0 for all calls to of_property_read_variable_u*_array > by of_property_read_u*_array, we basically know we'll get a buffer of > sz_min values but we don't actually make use of the higher bound > checking of of_find_property_value_of_size. > This was the original behaviour of the of_property_read_u*_array functions. If you look back at the of_property_read_u*_array implementations before my patch they passed max=0 to of_find_property_value_of_size. To avoid duplicating code I reimplemented the of_property_read_u*_array to use the new of_property_read_variable_u*_array hence they pass sz_max=0 to preserve the original behaviour that max=0 to of_find_property_value_of_size, so that I didn't break any code that might depend on that. > We could enforce this higher bound check by, instead of setting sz_max > to 0, setting sz_max to sz_min in calls to of_property_read_u*_array. > > But I guess there is a reason for sz_max being 0. Rob, Richard (commit > signer of this code) do you know why? Could you explain? > >> Anyway, this change still removes some bounds checking. Are they >> important? Do they need to be added back? >> > > The edge-slowdown and the vddmac values are compared against a const > array so we´re fine with those ones. > > For the led-X-mode, I added a constant for supported modes that gets > checked when retrieving the DT property. So we´re fine here too. > > Quentin >
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c index 49dc23117732..3c7b02bb5c38 100644 --- a/drivers/net/phy/mscc.c +++ b/drivers/net/phy/mscc.c @@ -129,7 +129,7 @@ struct vsc8531_private { #ifdef CONFIG_OF_MDIO struct vsc8531_edge_rate_table { u32 vddmac; - u8 slowdown[8]; + u32 slowdown[8]; }; static const struct vsc8531_edge_rate_table edge_table[] = { @@ -386,8 +386,7 @@ static void vsc85xx_wol_get(struct phy_device *phydev, #ifdef CONFIG_OF_MDIO static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) { - u8 sd; - u32 vdd; + u32 vdd, sd; int rc, i, j; struct device *dev = &phydev->mdio.dev; struct device_node *of_node = dev->of_node; @@ -400,7 +399,7 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) if (rc != 0) vdd = MSCC_VDDMAC_3300; - rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown", &sd); + rc = of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd); if (rc != 0) sd = 0;
In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown' is an u8, even though it's read as an u8 in the driver. Let's update the driver to take into consideration that the 'vsc8531,edge-slowdown' property is of the default type u32. Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> --- added in v2 drivers/net/phy/mscc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)