netdev - Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f708ca1f-6121-495a-a2af-bc725c04392f@intel.com>
Date: Fri, 5 Jul 2024 14:37:58 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Horman <horms@...nel.org>
CC: Michael Chan <michael.chan@...adcom.com>, <netdev@...r.kernel.org>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 1/3] bnxt_en: check for fw_ver_str truncation

On 7/5/24 13:26, Simon Horman wrote:
> Given the sizes of the buffers involved, it is theoretically
> possible for fw_ver_str to be truncated. Detect this and
> stop ethtool initialisation if this occurs.
> 
> Flagged by gcc-14:
> 
>    .../bnxt_ethtool.c: In function 'bnxt_ethtool_init':
>    drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=]
>     4144 |                          "/pkg %s", buf);
>          |                                ^~   ~~~

gcc is right, and you are right that we don't want such warnings
but I believe that the current flow is fine (copy as much as possible,
then proceed)

>    In function 'bnxt_get_pkgver',
>        inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3:
>    .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31
>     4143 |                 snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     4144 |                          "/pkg %s", buf);
>          |                          ~~~~~~~~~~~~~~~
> 
> Compile tested only.
> 
> Signed-off-by: Simon Horman <horms@...nel.org>
> ---
> It appears to me that size is underestimated by 1 byte -
> it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1,
> because the size argument to snprintf should include the space for the
> trailing '\0'. But I have not changed that as it is separate from
> the issue this patch addresses.

you are addressing "bad size" for copying strings around, I will just
fix that part too

> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index bf157f6cc042..5ccc3cc4ba7d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size)
>   	return rc;
>   }
>   
> -static void bnxt_get_pkgver(struct net_device *dev)
> +static int bnxt_get_pkgver(struct net_device *dev)
>   {
>   	struct bnxt *bp = netdev_priv(dev);
>   	char buf[FW_VER_STR_LEN];
> -	int len;
>   
>   	if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) {
> -		len = strlen(bp->fw_ver_str);
> -		snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1,
> -			 "/pkg %s", buf);
> +		int offset, size, rc;
> +
> +		offset = strlen(bp->fw_ver_str);
> +		size = FW_VER_STR_LEN - offset - 1;
> +
> +		rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf);
> +		if (rc >= size)
> +			return -E2BIG;

On error I would just replace last few bytes with "(...)" or "...", or
even "~". Other option is to enlarge bp->fw_ver_str, but I have not
looked there.

>   	}
> +
> +	return 0;
>   }
>   
>   static int bnxt_get_eeprom(struct net_device *dev,
> @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp)
>   	struct net_device *dev = bp->dev;
>   	int i, rc;
>   
> -	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER))
> -		bnxt_get_pkgver(dev);
> +	if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) {
> +		rc = bnxt_get_pkgver(dev);
> +		if (rc)
> +			return;

and here you are changing the flow, I would like to still init the
rest of the bnxt' ethtool stuff despite one informative string
being turncated

> +	}
>   
>   	bp->num_tests = 0;
>   	if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
>