[PATCH net v2 0/2] net: phy: Fix potential string cut when using PHY_ID_FMT

[PATCH net v2 0/2] net: phy: Fix potential string cut when using PHY_ID_FMT

[Andy Shevchenko]

The agreement and also PHY_MAX_ADDR limit suggests that the PHY id can't occupy
more than 2 hex digits. In some cases GCC complains about potential string cut.
Avoid that by limiting specifier to print only 2 hex digits (patch 1).
With that, fix the ASIX driver that triggers GCC and use agreed + 3 in the size
of the phy_name (patch 2).

In v2:
- added first patch
- added a conditional to the ASIX driver (Andrew)

Andy Shevchenko (2):
  net: phy: Fix formatting specifier to avoid potential string cuts
  net: usb: asix: ax88772: Increase phy_name size

 drivers/net/usb/ax88172a.c | 9 ++++++---
 include/linux/phy.h        | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.47.2

[PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[Andy Shevchenko]

The PHY_ID_FMT is defined with '%02x' which is _minumum_ digits
to be printed. This, in particular, may trigger GCC warning, when
the parameter for the above mentioned specifier is bigger than
a byte. Avoid this, by limiting the amount of digits to be printed
to two. This is okay as the PHY maximum address is 31 and it fits.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/phy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 19f076a71f94..3b18c241f33e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -309,7 +309,7 @@ static inline long rgmii_clock(int speed)
 #define PHY_MAX_ADDR	32
 
 /* Used when trying to connect to a specific phy (mii bus id:phy device id) */
-#define PHY_ID_FMT "%s:%02x"
+#define PHY_ID_FMT "%s:%02hhx"
 
 #define MII_BUS_ID_SIZE	61
 
-- 
2.47.2

[PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Andy Shevchenko]

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(-)

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,
-- 
2.47.2

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[Andy Shevchenko]

On Wed, Mar 19, 2025 at 12:54:33PM +0200, Andy Shevchenko wrote:
> The PHY_ID_FMT is defined with '%02x' which is _minumum_ digits
> to be printed. This, in particular, may trigger GCC warning, when
> the parameter for the above mentioned specifier is bigger than
> a byte. Avoid this, by limiting the amount of digits to be printed
> to two. This is okay as the PHY maximum address is 31 and it fits.

For the curious: additional reading in the commit
46d57a7a8e33 ("docs: printk-formats: Fix hex printing of signed values").

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Andy Shevchenko]

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.

> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-phy-Fix-formatting-specifier-to-avoid-potential-string-cuts/20250319-190433
base:   net/main
patch link:    https://lore.kernel.org/r/20250319105813.3102076-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192340.iVN44lM2-lkp@intel.com/config)
compiler: clang version 20.1.0 (https://github.com/llvm/llvm-project 24a30daaa559829ad079f2ff7f73eb4e18095f88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192340.iVN44lM2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503192340.iVN44lM2-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/usb/ax88172a.c:312:20: warning: format specifies type 'unsigned char' but the argument has type 'u16' (aka 'unsigned short') [-Wformat]
     311 |         snprintf(priv->phy_name, 20, PHY_ID_FMT,
         |                                      ~~~~~~~~~~
     312 |                  priv->mdio->id, priv->phy_addr);
         |                                  ^~~~~~~~~~~~~~
   1 warning generated.


vim +312 drivers/net/usb/ax88172a.c

16626b0cc3d5af Christian Riesch 2012-07-13  260  
16626b0cc3d5af Christian Riesch 2012-07-13  261  static int ax88172a_reset(struct usbnet *dev)
16626b0cc3d5af Christian Riesch 2012-07-13  262  {
16626b0cc3d5af Christian Riesch 2012-07-13  263  	struct asix_data *data = (struct asix_data *)&dev->data;
16626b0cc3d5af Christian Riesch 2012-07-13  264  	struct ax88172a_private *priv = dev->driver_priv;
16626b0cc3d5af Christian Riesch 2012-07-13  265  	int ret;
16626b0cc3d5af Christian Riesch 2012-07-13  266  	u16 rx_ctl;
16626b0cc3d5af Christian Riesch 2012-07-13  267  
16626b0cc3d5af Christian Riesch 2012-07-13  268  	ax88172a_reset_phy(dev, priv->use_embdphy);
16626b0cc3d5af Christian Riesch 2012-07-13  269  
16626b0cc3d5af Christian Riesch 2012-07-13  270  	msleep(150);
d9fe64e511144c Robert Foss      2016-08-29  271  	rx_ctl = asix_read_rx_ctl(dev, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  272  	netdev_dbg(dev->net, "RX_CTL is 0x%04x after software reset\n", rx_ctl);
d9fe64e511144c Robert Foss      2016-08-29  273  	ret = asix_write_rx_ctl(dev, 0x0000, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  274  	if (ret < 0)
16626b0cc3d5af Christian Riesch 2012-07-13  275  		goto out;
16626b0cc3d5af Christian Riesch 2012-07-13  276  
d9fe64e511144c Robert Foss      2016-08-29  277  	rx_ctl = asix_read_rx_ctl(dev, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  278  	netdev_dbg(dev->net, "RX_CTL is 0x%04x setting to 0x0000\n", rx_ctl);
16626b0cc3d5af Christian Riesch 2012-07-13  279  
16626b0cc3d5af Christian Riesch 2012-07-13  280  	msleep(150);
16626b0cc3d5af Christian Riesch 2012-07-13  281  
16626b0cc3d5af Christian Riesch 2012-07-13  282  	ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
16626b0cc3d5af Christian Riesch 2012-07-13  283  			     AX88772_IPG0_DEFAULT | AX88772_IPG1_DEFAULT,
d9fe64e511144c Robert Foss      2016-08-29  284  			     AX88772_IPG2_DEFAULT, 0, NULL, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  285  	if (ret < 0) {
16626b0cc3d5af Christian Riesch 2012-07-13  286  		netdev_err(dev->net, "Write IPG,IPG1,IPG2 failed: %d\n", ret);
16626b0cc3d5af Christian Riesch 2012-07-13  287  		goto out;
16626b0cc3d5af Christian Riesch 2012-07-13  288  	}
16626b0cc3d5af Christian Riesch 2012-07-13  289  
16626b0cc3d5af Christian Riesch 2012-07-13  290  	/* Rewrite MAC address */
16626b0cc3d5af Christian Riesch 2012-07-13  291  	memcpy(data->mac_addr, dev->net->dev_addr, ETH_ALEN);
16626b0cc3d5af Christian Riesch 2012-07-13  292  	ret = asix_write_cmd(dev, AX_CMD_WRITE_NODE_ID, 0, 0, ETH_ALEN,
d9fe64e511144c Robert Foss      2016-08-29  293  			     data->mac_addr, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  294  	if (ret < 0)
16626b0cc3d5af Christian Riesch 2012-07-13  295  		goto out;
16626b0cc3d5af Christian Riesch 2012-07-13  296  
16626b0cc3d5af Christian Riesch 2012-07-13  297  	/* Set RX_CTL to default values with 2k buffer, and enable cactus */
d9fe64e511144c Robert Foss      2016-08-29  298  	ret = asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  299  	if (ret < 0)
16626b0cc3d5af Christian Riesch 2012-07-13  300  		goto out;
16626b0cc3d5af Christian Riesch 2012-07-13  301  
d9fe64e511144c Robert Foss      2016-08-29  302  	rx_ctl = asix_read_rx_ctl(dev, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  303  	netdev_dbg(dev->net, "RX_CTL is 0x%04x after all initializations\n",
16626b0cc3d5af Christian Riesch 2012-07-13  304  		   rx_ctl);
16626b0cc3d5af Christian Riesch 2012-07-13  305  
d9fe64e511144c Robert Foss      2016-08-29  306  	rx_ctl = asix_read_medium_status(dev, 0);
16626b0cc3d5af Christian Riesch 2012-07-13  307  	netdev_dbg(dev->net, "Medium Status is 0x%04x after all initializations\n",
16626b0cc3d5af Christian Riesch 2012-07-13  308  		   rx_ctl);
16626b0cc3d5af Christian Riesch 2012-07-13  309  
16626b0cc3d5af Christian Riesch 2012-07-13  310  	/* Connect to PHY */
16626b0cc3d5af Christian Riesch 2012-07-13  311  	snprintf(priv->phy_name, 20, PHY_ID_FMT,
16626b0cc3d5af Christian Riesch 2012-07-13 @312  		 priv->mdio->id, priv->phy_addr);
16626b0cc3d5af Christian Riesch 2012-07-13  313  
16626b0cc3d5af Christian Riesch 2012-07-13  314  	priv->phydev = phy_connect(dev->net, priv->phy_name,
16626b0cc3d5af Christian Riesch 2012-07-13  315  				   &ax88172a_adjust_link,
f9a8f83b04e0c3 Florian Fainelli 2013-01-14  316  				   PHY_INTERFACE_MODE_MII);
16626b0cc3d5af Christian Riesch 2012-07-13  317  	if (IS_ERR(priv->phydev)) {
16626b0cc3d5af Christian Riesch 2012-07-13  318  		netdev_err(dev->net, "Could not connect to PHY device %s\n",
16626b0cc3d5af Christian Riesch 2012-07-13  319  			   priv->phy_name);
16626b0cc3d5af Christian Riesch 2012-07-13  320  		ret = PTR_ERR(priv->phydev);
16626b0cc3d5af Christian Riesch 2012-07-13  321  		goto out;
16626b0cc3d5af Christian Riesch 2012-07-13  322  	}
16626b0cc3d5af Christian Riesch 2012-07-13  323  
16626b0cc3d5af Christian Riesch 2012-07-13  324  	netdev_info(dev->net, "Connected to phy %s\n", priv->phy_name);
16626b0cc3d5af Christian Riesch 2012-07-13  325  
16626b0cc3d5af Christian Riesch 2012-07-13  326  	/* During power-up, the AX88172A set the power down (BMCR_PDOWN)
16626b0cc3d5af Christian Riesch 2012-07-13  327  	 * bit of the PHY. Bring the PHY up again.
16626b0cc3d5af Christian Riesch 2012-07-13  328  	 */
16626b0cc3d5af Christian Riesch 2012-07-13  329  	genphy_resume(priv->phydev);
16626b0cc3d5af Christian Riesch 2012-07-13  330  	phy_start(priv->phydev);
16626b0cc3d5af Christian Riesch 2012-07-13  331  
16626b0cc3d5af Christian Riesch 2012-07-13  332  	return 0;
16626b0cc3d5af Christian Riesch 2012-07-13  333  
16626b0cc3d5af Christian Riesch 2012-07-13  334  out:
16626b0cc3d5af Christian Riesch 2012-07-13  335  	return ret;
16626b0cc3d5af Christian Riesch 2012-07-13  336  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 12:54:33PM +0200, Andy Shevchenko wrote:
> -#define PHY_ID_FMT "%s:%02x"
> +#define PHY_ID_FMT "%s:%02hhx"

I was going to state whether it is correct to use hh with an "int"
argument, as printf() suggests its only for use with arguments of
type 'signed char' and 'unsigned char'. My suspicion has been
confirmed by the warning the kbuild bot has just reported.

It seems this is not a correct fix for the problem you report.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[Andy Shevchenko]

On Wed, Mar 19, 2025 at 11:37:19PM +0800, kernel test robot wrote:
> Hi Andy,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on net/main]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-phy-Fix-formatting-specifier-to-avoid-potential-string-cuts/20250319-190433
> base:   net/main
> patch link:    https://lore.kernel.org/r/20250319105813.3102076-2-andriy.shevchenko%40linux.intel.com
> patch subject: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts
> config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192340.iVN44lM2-lkp@intel.com/config)
> compiler: clang version 20.1.0 (https://github.com/llvm/llvm-project 24a30daaa559829ad079f2ff7f73eb4e18095f88)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192340.iVN44lM2-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503192340.iVN44lM2-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/net/usb/ax88172a.c:312:20: warning: format specifies type 'unsigned char' but the argument has type 'u16' (aka 'unsigned short') [-Wformat]
>      311 |         snprintf(priv->phy_name, 20, PHY_ID_FMT,
>          |                                      ~~~~~~~~~~
>      312 |                  priv->mdio->id, priv->phy_addr);
>          |                                  ^~~~~~~~~~~~~~
>    1 warning generated.

It's fun: while working around GCC complain, clang does not agree with... :-)
Let's wait more, probably the simplest fix (if this becomes only a single user
of the non-byte parameter) is to move it to u8.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[Andy Shevchenko]

On Wed, Mar 19, 2025 at 03:46:48PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 19, 2025 at 12:54:33PM +0200, Andy Shevchenko wrote:

...

> > -#define PHY_ID_FMT "%s:%02x"
> > +#define PHY_ID_FMT "%s:%02hhx"
> 
> I was going to state whether it is correct to use hh with an "int"
> argument, as printf() suggests its only for use with arguments of
> type 'signed char' and 'unsigned char'. My suspicion has been

Yeah...

> It seems this is not a correct fix for the problem you report.

Apparently looks like (from the clang point of view — I compiled it with GCC).

...

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Russell King (Oracle)]

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-phy-Fix-formatting-specifier-to-avoid-potential-string-cuts/20250319-190433
base:   net/main
patch link:    https://lore.kernel.org/r/20250319105813.3102076-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH net v2 1/2] net: phy: Fix formatting specifier to avoid potential string cuts
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20250320/202503200024.WkseT3sA-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250320/202503200024.WkseT3sA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503200024.WkseT3sA-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/broadcom/genet/bcmmii.c:17:
   drivers/net/ethernet/broadcom/genet/bcmmii.c: In function 'bcmgenet_mii_pd_init':
>> include/linux/phy.h:312:20: warning: '%02hhx' directive output may be truncated writing 2 bytes into a region of size between 0 and 60 [-Wformat-truncation=]
     312 | #define PHY_ID_FMT "%s:%02hhx"
         |                    ^~~~~~~~~~~
   drivers/net/ethernet/broadcom/genet/bcmmii.c:604:53: note: in expansion of macro 'PHY_ID_FMT'
     604 |                 snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT,
         |                                                     ^~~~~~~~~~
   include/linux/phy.h:312:24: note: format string is defined here
     312 | #define PHY_ID_FMT "%s:%02hhx"
         |                        ^~~~~~
   include/linux/phy.h:312:20: note: using the range [0, 255] for directive argument
     312 | #define PHY_ID_FMT "%s:%02hhx"
         |                    ^~~~~~~~~~~
   drivers/net/ethernet/broadcom/genet/bcmmii.c:604:53: note: in expansion of macro 'PHY_ID_FMT'
     604 |                 snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT,
         |                                                     ^~~~~~~~~~
   drivers/net/ethernet/broadcom/genet/bcmmii.c:604:17: note: 'snprintf' output between 4 and 64 bytes into a destination of size 61
     604 |                 snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT,
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     605 |                          mdio_bus_id, pd->phy_address);
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   loongarch64-linux-ld: arch/loongarch/kernel/head.o: relocation R_LARCH_B26 overflow 0xfffffffff3911f74
   arch/loongarch/kernel/head.o: in function `smpboot_entry':
>> (.ref.text+0x15c): relocation truncated to fit: R_LARCH_B26 against symbol `start_secondary' defined in .text section in arch/loongarch/kernel/smp.o
   loongarch64-linux-ld: final link failed: bad value


vim +312 include/linux/phy.h

   310	
   311	/* Used when trying to connect to a specific phy (mii bus id:phy device id) */
 > 312	#define PHY_ID_FMT "%s:%02hhx"
   313	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net v2 2/2] net: usb: asix: ax88772: Increase phy_name size

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko