| Message ID | 20250319105813.3102076-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Fix potential string cut when using PHY_ID_FMT | expand |
On Wed, Mar 19, 2025 at 12:54:34PM +0200, Andy Shevchenko wrote: > GCC compiler (Debian 14.2.0-17) is not happy about printing > into a short buffer (when build with `make W=1`): > > drivers/net/usb/ax88172a.c: In function ‘ax88172a_reset’: > include/linux/phy.h:312:20: error: ‘%s’ directive output may be truncated writing up to 60 bytes into a region of size 20 [-Werror=format-truncation=] > > Indeed, the buffer size is chosen based on some assumptions, while > in general the assigned name might not fit. Increase the buffer to > cover maximum length of the parameters. With that, change snprintf() > to use sizeof() instead of hard coded number. ... > if (ret < 0) > goto free; > - > + if (ret >= PHY_MAX_ADDR) { > + netdev_err(dev->net, "Invalid PHY ID %x\n", ret); > + return -ENODEV; Oh, I was blindly put what had been suggested, needs a fix, but I will wait for other comments if any before issuing a v3. > + }
On Wed, Mar 19, 2025 at 12:54:34PM +0200, Andy Shevchenko wrote: > GCC compiler (Debian 14.2.0-17) is not happy about printing > into a short buffer (when build with `make W=1`): > > drivers/net/usb/ax88172a.c: In function ‘ax88172a_reset’: > include/linux/phy.h:312:20: error: ‘%s’ directive output may be truncated writing up to 60 bytes into a region of size 20 [-Werror=format-truncation=] GCC reckons this can be up to 60 bytes... > struct ax88172a_private { > struct mii_bus *mdio; > struct phy_device *phydev; > - char phy_name[20]; > + char phy_name[MII_BUS_ID_SIZE + 3]; MII_BUS_ID_SIZE is sized to 61, and is what is used in struct mii_bus::id. Why there a +3 here, which seems like a random constant to make it 64-bit aligned in size. If we have need to increase MII_BUS_ID_SIZE in the future, this kind of alignment then goes wrong... If the intention is to align it to 64-bit then there's surely a better and future-proof ways to do that. I'm also surprised that the +3 randomness wasn't described in the commit message. > @@ -210,7 +210,10 @@ static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf) > ret = asix_read_phy_addr(dev, priv->use_embdphy); > if (ret < 0) > goto free; > - > + if (ret >= PHY_MAX_ADDR) { > + netdev_err(dev->net, "Invalid PHY ID %x\n", ret); An address is not a "PHY ID". "Invalid PHY address %d\n" probably makes more sense, but if you want to keep the hex, then it really should be %#x or 0x%x to make it clear that e.g. "20" is hex and not decimal.
On Wed, Mar 19, 2025 at 02:43:40PM +0000, Russell King (Oracle) wrote: > On Wed, Mar 19, 2025 at 12:54:34PM +0200, Andy Shevchenko wrote: > > GCC compiler (Debian 14.2.0-17) is not happy about printing > > into a short buffer (when build with `make W=1`): > > > > drivers/net/usb/ax88172a.c: In function ‘ax88172a_reset’: > > include/linux/phy.h:312:20: error: ‘%s’ directive output may be truncated writing up to 60 bytes into a region of size 20 [-Werror=format-truncation=] > > GCC reckons this can be up to 60 bytes... It has two complains, but they a bit differ, first one was about %s, as you see above, the other one which I missed to add here is about %02x: drivers/net/usb/ax88172a.c:311:9: note: ‘snprintf’ output between 4 and 66 bytes into a destination of size 20 ... > > - char phy_name[20]; > > + char phy_name[MII_BUS_ID_SIZE + 3]; > > MII_BUS_ID_SIZE is sized to 61, and is what is used in struct > mii_bus::id. Why there a +3 here, which seems like a random constant to > make it 64-bit aligned in size. If we have need to increase > MII_BUS_ID_SIZE in the future, this kind of alignment then goes > wrong... > > If the intention is to align it to 64-bit then there's surely a better > and future-proof ways to do that. Nope, intention is to cover the rest after %s. ... > I'm also surprised that the +3 randomness wasn't described in the > commit message. It was referred in the cover letter and previous discussion, but I agree that it has to be clarified here, it's ':%02x', the %s case, i.e. MII_BUS_ID_SIZE covers 60 characters + NUL. ... > > + if (ret >= PHY_MAX_ADDR) { > > + netdev_err(dev->net, "Invalid PHY ID %x\n", ret); > > An address is not a "PHY ID". "Invalid PHY address %d\n" probably makes > more sense, but if you want to keep the hex, then it really should be > %#x or 0x%x to make it clear that e.g. "20" is hex and not decimal. Sure, I fix it locally, but we need to understand how to go with the +3/+whatever fix, so format specifier won't hit us back. ... Thank you for the review!
On Wed, Mar 19, 2025 at 06:22:54PM +0200, Andy Shevchenko wrote: > On Wed, Mar 19, 2025 at 02:43:40PM +0000, Russell King (Oracle) wrote: > > On Wed, Mar 19, 2025 at 12:54:34PM +0200, Andy Shevchenko wrote: > > > - char phy_name[20]; > > > + char phy_name[MII_BUS_ID_SIZE + 3]; > > > > MII_BUS_ID_SIZE is sized to 61, and is what is used in struct > > mii_bus::id. Why there a +3 here, which seems like a random constant to > > make it 64-bit aligned in size. If we have need to increase > > MII_BUS_ID_SIZE in the future, this kind of alignment then goes > > wrong... > > > > If the intention is to align it to 64-bit then there's surely a better > > and future-proof ways to do that. > > Nope, intention is to cover the rest after %s. Oops, I had missed that MII_BUS_ID_SIZE is the size of the "%s" part. I think linux/phy.h should declare: #define PHY_ID_SIZE (MII_BUS_ID_SIZE + 3) to cater for the ":XX" that PHY_ID_FMT adds. So the above would become: char phy_name[PHY_ID_SIZE]; I wonder whether keeping PHY_ID_FMT as-is, but casting the argument to a u8 would solve the issue? Maybe something like: static inline void phy_format_id(char *dst, size_t n, const char *mii_bus_id, u8 phy_dev_id) { BUILD_BUG_ON_MSG(n < PHY_ID_SIZE, "PHY ID destination too small"); snprintf(dat, n, PHY_ID_FMT, mii_bus_id, phy_dev_id); } would solve it?
Wed, Mar 19, 2025 at 05:13:52PM +0000, Russell King (Oracle) kirjoitti: > On Wed, Mar 19, 2025 at 06:22:54PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 19, 2025 at 02:43:40PM +0000, Russell King (Oracle) wrote: > > > On Wed, Mar 19, 2025 at 12:54:34PM +0200, Andy Shevchenko wrote: > > > > - char phy_name[20]; > > > > + char phy_name[MII_BUS_ID_SIZE + 3]; > > > > > > MII_BUS_ID_SIZE is sized to 61, and is what is used in struct > > > mii_bus::id. Why there a +3 here, which seems like a random constant to > > > make it 64-bit aligned in size. If we have need to increase > > > MII_BUS_ID_SIZE in the future, this kind of alignment then goes > > > wrong... > > > > > > If the intention is to align it to 64-bit then there's surely a better > > > and future-proof ways to do that. > > > > Nope, intention is to cover the rest after %s. > > Oops, I had missed that MII_BUS_ID_SIZE is the size of the "%s" part. > I think linux/phy.h should declare: > > #define PHY_ID_SIZE (MII_BUS_ID_SIZE + 3) > > to cater for the ":XX" that PHY_ID_FMT adds. > > So the above would become: > > char phy_name[PHY_ID_SIZE]; > > I wonder whether keeping PHY_ID_FMT as-is, but casting the argument > to a u8 would solve the issue? > > Maybe something like: > > static inline void > phy_format_id(char *dst, size_t n, const char *mii_bus_id, u8 phy_dev_id) > { > BUILD_BUG_ON_MSG(n < PHY_ID_SIZE, "PHY ID destination too small"); > snprintf(dat, n, PHY_ID_FMT, mii_bus_id, phy_dev_id); > } > > would solve it? Would you like to send a formal patch? I will base my fix on top of it and test that in my case.
diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c index e47bb125048d..df00c62dd538 100644 --- a/drivers/net/usb/ax88172a.c +++ b/drivers/net/usb/ax88172a.c @@ -18,7 +18,7 @@ struct ax88172a_private { struct mii_bus *mdio; struct phy_device *phydev; - char phy_name[20]; + char phy_name[MII_BUS_ID_SIZE + 3]; u16 phy_addr; u16 oldmode; int use_embdphy; @@ -210,7 +210,10 @@ static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf) ret = asix_read_phy_addr(dev, priv->use_embdphy); if (ret < 0) goto free; - + if (ret >= PHY_MAX_ADDR) { + netdev_err(dev->net, "Invalid PHY ID %x\n", ret); + return -ENODEV; + } priv->phy_addr = ret; ax88172a_reset_phy(dev, priv->use_embdphy); @@ -308,7 +311,7 @@ static int ax88172a_reset(struct usbnet *dev) rx_ctl); /* Connect to PHY */ - snprintf(priv->phy_name, 20, PHY_ID_FMT, + snprintf(priv->phy_name, sizeof(priv->phy_name), PHY_ID_FMT, priv->mdio->id, priv->phy_addr); priv->phydev = phy_connect(dev->net, priv->phy_name,
GCC compiler (Debian 14.2.0-17) is not happy about printing into a short buffer (when build with `make W=1`): drivers/net/usb/ax88172a.c: In function ‘ax88172a_reset’: include/linux/phy.h:312:20: error: ‘%s’ directive output may be truncated writing up to 60 bytes into a region of size 20 [-Werror=format-truncation=] Indeed, the buffer size is chosen based on some assumptions, while in general the assigned name might not fit. Increase the buffer to cover maximum length of the parameters. With that, change snprintf() to use sizeof() instead of hard coded number. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/usb/ax88172a.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)