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

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

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 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!

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?