| 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
| ||
|
Message-ID: <d7cj5kz2xnm2wg475oqs6fyc77l2aictw5gyq7n5oogp3g4pii@bibogucg42s3>
Date: Thu, 17 Apr 2025 14:09:18 +0000
From: Dragos Tatulea <dtatulea@...dia.com>
To: Kees Cook <kees@...nel.org>, Saeed Mahameed <saeedm@...dia.com>
Cc: Tariq Toukan <tariqt@...dia.com>, Leon Romanovsky <leon@...nel.org>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>,
netdev@...r.kernel.org, linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH] net/mlx5e: ethtool: Fix formatting of
ptp_rq0_csum_complete_tail_slow
On Tue, Apr 15, 2025 at 07:01:14PM -0700, Kees Cook wrote:
> The new GCC 15 warning -Wunterminated-string-initialization reports:
>
> In file included from drivers/net/ethernet/mellanox/mlx5/core/en.h:55,
> from drivers/net/ethernet/mellanox/mlx5/core/en_stats.c:34:
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.h:57:46: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (33 chars into 32 available) [-Wunterminated-string-initialization]
> 57 | #define MLX5E_DECLARE_PTP_RQ_STAT(type, fld) "ptp_rq%d_"#fld, offsetof(type, fld)
> | ^~~~~~~~~~~
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.c:2279:11: note: in expansion of macro 'MLX5E_DECLARE_PTP_RQ_STAT'
> 2279 | { MLX5E_DECLARE_PTP_RQ_STAT(struct mlx5e_rq_stats, csum_complete_tail_slow) },
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> This stat string is being used in ethtool_sprintf(), so it must be a
> valid NUL-terminated string. Currently the string lacks the final NUL
> byte (as GCC warns), but by absolute luck, the next byte in memory is a
> space (decimal 32) followed by a NUL. "format" is immediately followed
> by little-endian size_t:
>
> struct counter_desc {
> char format[32]; /* 0 32 */
> size_t offset; /* 32 8 */
> };
>
> The "offset" member is populated by the stats member offset:
>
> #define MLX5E_DECLARE_PTP_RQ_STAT(type, fld) "ptp_rq%d_"#fld, offsetof(type, fld)
>
> which for this struct mlx5e_rq_stats member, csum_complete_tail_slow, is
> 32, or space, and then the rest of the "offset" bytes are NULs.
>
> struct mlx5e_rq_stats {
> ...
> u64 csum_complete_tail_slow; /* 32 8 */
>
> The use of vsnprintf(), within ethtool_sprintf(), reads past the end of
> "format" and sees the format string as "ptp_rq%d_csum_complete_tail_slow ",
> with %d getting resolved by MLX5E_PTP_CHANNEL_IX (value 0):
>
> ethtool_sprintf(data, ptp_rq_stats_desc[i].format,
> MLX5E_PTP_CHANNEL_IX);
>
> With an output result of "ptp_rq0_csum_complete_tail_slow", which gets
> precisely truncated to 31 characters with a trailing NUL.
>
> So, instead of accidentally getting this correct due to the NUL bytes
> at the end of the size_t that happens to follow the format string, just
> make the string initializer 1 byte shorter by replacing "%d" with "0",
> since MLX5E_PTP_CHANNEL_IX is already hard-coded. This results in no
> initializer truncation and no need to call sprintf().
>
Thanks for the detailed write-up!
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---
> Cc: Saeed Mahameed <saeedm@...dia.com>
> Cc: Tariq Toukan <tariqt@...dia.com>
> Cc: Leon Romanovsky <leon@...nel.org>
> Cc: Andrew Lunn <andrew+netdev@...n.ch>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Richard Cochran <richardcochran@...il.com>
> Cc: netdev@...r.kernel.org
> Cc: linux-rdma@...r.kernel.org
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 3 +--
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index 1c121b435016..19664fa7f217 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -2424,8 +2424,7 @@ static MLX5E_DECLARE_STATS_GRP_OP_FILL_STRS(ptp)
> }
> if (priv->rx_ptp_opened) {
> for (i = 0; i < NUM_PTP_RQ_STATS; i++)
> - ethtool_sprintf(data, ptp_rq_stats_desc[i].format,
> - MLX5E_PTP_CHANNEL_IX);
> + ethtool_puts(data, ptp_rq_stats_desc[i].format);
> }
> }
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> index 8de6fcbd3a03..def5dea1463d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> @@ -54,7 +54,7 @@
> #define MLX5E_DECLARE_PTP_TX_STAT(type, fld) "ptp_tx%d_"#fld, offsetof(type, fld)
> #define MLX5E_DECLARE_PTP_CH_STAT(type, fld) "ptp_ch_"#fld, offsetof(type, fld)
> #define MLX5E_DECLARE_PTP_CQ_STAT(type, fld) "ptp_cq%d_"#fld, offsetof(type, fld)
> -#define MLX5E_DECLARE_PTP_RQ_STAT(type, fld) "ptp_rq%d_"#fld, offsetof(type, fld)
> +#define MLX5E_DECLARE_PTP_RQ_STAT(type, fld) "ptp_rq0_"#fld, offsetof(type, fld)
>
> #define MLX5E_DECLARE_QOS_TX_STAT(type, fld) "qos_tx%d_"#fld, offsetof(type, fld)
>
> --
> 2.34.1
>
>
Thought of renaming the stat but then it will impact current users which
is worse: csum_complete_tail_slow is a regular rq stat as well. So:
Reviewed-by: Dragos Tatulea <dtatulea@...dia.com>
Thanks,
Dragos
Powered by blists - more mailing lists