| Message ID | 20140305003544.23016.55079.stgit@bhelgaas-glaptop.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
2014-03-04 16:35 GMT-08:00 Bjorn Helgaas <bhelgaas@google.com>: > With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the > settings[] "array subscript is above array bounds", I think because idx is > a signed integer and if the caller supplied idx < 0, we pass the guard but > still reference out of bounds. > > Fix this by making idx unsigned here and elsewhere. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/phy/phy.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 19c9eca0ef26..76d96b9ebcdb 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -164,9 +164,9 @@ static const struct phy_setting settings[] = { > * of that setting. Returns the index of the last setting if > * none of the others match. > */ > -static inline int phy_find_setting(int speed, int duplex) > +static inline unsigned int phy_find_setting(int speed, int duplex) > { > - int idx = 0; > + unsigned int idx = 0; > > while (idx < ARRAY_SIZE(settings) && > (settings[idx].speed != speed || settings[idx].duplex != duplex)) > @@ -185,7 +185,7 @@ static inline int phy_find_setting(int speed, int duplex) > * the mask in features. Returns the index of the last setting > * if nothing else matches. > */ > -static inline int phy_find_valid(int idx, u32 features) > +static inline unsigned int phy_find_valid(unsigned int idx, u32 features) > { > while (idx < MAX_NUM_SETTINGS && !(settings[idx].setting & features)) > idx++; > @@ -204,7 +204,7 @@ static inline int phy_find_valid(int idx, u32 features) > static void phy_sanitize_settings(struct phy_device *phydev) > { > u32 features = phydev->supported; > - int idx; > + unsigned int idx; > > /* Sanitize settings based on PHY capabilities */ > if ((features & SUPPORTED_Autoneg) == 0) > @@ -954,7 +954,8 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable) > (phydev->interface == PHY_INTERFACE_MODE_RGMII))) { > int eee_lp, eee_cap, eee_adv; > u32 lp, cap, adv; > - int idx, status; > + int status; > + unsigned int idx; > > /* Read phy status to properly get the right settings */ > status = phy_read_status(phydev); > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bjorn Helgaas > With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the > settings[] "array subscript is above array bounds", I think because idx is > a signed integer and if the caller supplied idx < 0, we pass the guard but > still reference out of bounds. Not rejecting the patch but... Just indexing an array with 'int' shouldn't cause this warning, so somewhere a caller must actually be passing an idx < 0. While changing the type to unsigned will make the comparison against the array bound reject the -1, I suspect that the specific call path didn't really intend passing a hard-coded -1. It might be worth trying to locate the call site that passes -1. David
On Wed, Mar 5, 2014 at 2:10 AM, David Laight <David.Laight@aculab.com> wrote: > From: Bjorn Helgaas >> With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the >> settings[] "array subscript is above array bounds", I think because idx is >> a signed integer and if the caller supplied idx < 0, we pass the guard but >> still reference out of bounds. > > Not rejecting the patch but... > > Just indexing an array with 'int' shouldn't cause this warning, > so somewhere a caller must actually be passing an idx < 0. > > While changing the type to unsigned will make the comparison > against the array bound reject the -1, I suspect that the > specific call path didn't really intend passing a hard-coded -1. > > It might be worth trying to locate the call site that passes -1. I agree 100%. If that's the case, we definitely should find that caller rather than apply this patch. I'll look more today. Bjorn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bjorn Helgaas > On Wed, Mar 5, 2014 at 2:10 AM, David Laight <David.Laight@aculab.com> wrote: > > From: Bjorn Helgaas > >> With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the > >> settings[] "array subscript is above array bounds", I think because idx is > >> a signed integer and if the caller supplied idx < 0, we pass the guard but > >> still reference out of bounds. > > > > Not rejecting the patch but... > > > > Just indexing an array with 'int' shouldn't cause this warning, > > so somewhere a caller must actually be passing an idx < 0. > > > > While changing the type to unsigned will make the comparison > > against the array bound reject the -1, I suspect that the > > specific call path didn't really intend passing a hard-coded -1. > > > > It might be worth trying to locate the call site that passes -1. > > I agree 100%. If that's the case, we definitely should find that > caller rather than apply this patch. I'll look more today. You might want to apply the patch as well :-) David
[+cc Florian] On Wed, Mar 5, 2014 at 2:10 AM, David Laight <David.Laight@aculab.com> wrote: > From: Bjorn Helgaas >> With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the >> settings[] "array subscript is above array bounds", I think because idx is >> a signed integer and if the caller supplied idx < 0, we pass the guard but >> still reference out of bounds. > > Not rejecting the patch but... > > Just indexing an array with 'int' shouldn't cause this warning, > so somewhere a caller must actually be passing an idx < 0. > > While changing the type to unsigned will make the comparison > against the array bound reject the -1, I suspect that the > specific call path didn't really intend passing a hard-coded -1. > > It might be worth trying to locate the call site that passes -1. I'm stumped. phy_find_valid() is static and only called from one place. The 'idx' argument is always the result of phy_find_setting(), which should always return something between 0 and ARRAY_SIZE(settings), so I don't see any way idx can be < 0. I stripped this down as far as I could; the resulting test code is at http://pastebin.com/pp1zMEWu if anybody else wants to look at it. I'm using gcc 4.8.x 20131105 (prerelease), with "-Warray-bounds -O2" flags. I hesitate to suspect a compiler bug, but it is very strange. For example, in my test code, replacing "MAX_NUM_SETTINGS" with "2" gets rid of the warnings. MAX_NUM_SETTINGS is known to be 2 at compile-time, so I don't know why this should make a difference. Bjorn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Bjorn Helgaas <bhelgaas@google.com> Date: Tue, 04 Mar 2014 17:35:44 -0700 > With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the > settings[] "array subscript is above array bounds", I think because idx is > a signed integer and if the caller supplied idx < 0, we pass the guard but > still reference out of bounds. > > Fix this by making idx unsigned here and elsewhere. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 19c9eca0ef26..76d96b9ebcdb 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -164,9 +164,9 @@ static const struct phy_setting settings[] = { * of that setting. Returns the index of the last setting if * none of the others match. */ -static inline int phy_find_setting(int speed, int duplex) +static inline unsigned int phy_find_setting(int speed, int duplex) { - int idx = 0; + unsigned int idx = 0; while (idx < ARRAY_SIZE(settings) && (settings[idx].speed != speed || settings[idx].duplex != duplex)) @@ -185,7 +185,7 @@ static inline int phy_find_setting(int speed, int duplex) * the mask in features. Returns the index of the last setting * if nothing else matches. */ -static inline int phy_find_valid(int idx, u32 features) +static inline unsigned int phy_find_valid(unsigned int idx, u32 features) { while (idx < MAX_NUM_SETTINGS && !(settings[idx].setting & features)) idx++; @@ -204,7 +204,7 @@ static inline int phy_find_valid(int idx, u32 features) static void phy_sanitize_settings(struct phy_device *phydev) { u32 features = phydev->supported; - int idx; + unsigned int idx; /* Sanitize settings based on PHY capabilities */ if ((features & SUPPORTED_Autoneg) == 0) @@ -954,7 +954,8 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable) (phydev->interface == PHY_INTERFACE_MODE_RGMII))) { int eee_lp, eee_cap, eee_adv; u32 lp, cap, adv; - int idx, status; + int status; + unsigned int idx; /* Read phy status to properly get the right settings */ status = phy_read_status(phydev);
With -Werror=array-bounds, gcc v4.7.x warns that in phy_find_valid(), the settings[] "array subscript is above array bounds", I think because idx is a signed integer and if the caller supplied idx < 0, we pass the guard but still reference out of bounds. Fix this by making idx unsigned here and elsewhere. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/net/phy/phy.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html