netdev - Re: [net V2 14/15] net/mlx5e: Check return value of snprintf writing to fw

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] [day] [month] [year] [list]

Message-ID: <87v89xbmlz.fsf@nvidia.com>
Date: Sun, 19 Nov 2023 10:54:00 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: David Laight <David.Laight@...LAB.COM>
Cc: 'Saeed Mahameed' <saeed@...nel.org>,  "David S. Miller"
 <davem@...emloft.net>,  Jakub Kicinski <kuba@...nel.org>,  Paolo Abeni
 <pabeni@...hat.com>,  Eric Dumazet <edumazet@...gle.com>,  Saeed Mahameed
 <saeedm@...dia.com>,  "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
  Tariq Toukan <tariqt@...dia.com>,  Dragos Tatulea <dtatulea@...dia.com>
Subject: Re: [net V2 14/15] net/mlx5e: Check return value of snprintf
 writing to fw_version buffer

On Sun, 19 Nov, 2023 10:46:57 +0000 David Laight <David.Laight@...LAB.COM> wrote:
> From: Saeed Mahameed
>> Sent: 14 November 2023 21:59
>> 
>> Treat the operation as an error case when the return value is equivalent to
>> the size of the name buffer. Failed to write null terminator to the name
>> buffer, making the string malformed and should not be used. Provide a
>> string with only the firmware version when forming the string with the
>> board id fails.
>
> Nak.
>
> RTFM snprintf().
>
>> 
>> Without check, will trigger -Wformat-truncation with W=1.
>> 
>>     drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: In function 'mlx5e_ethtool_get_drvinfo':
>>     drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:49:31: warning: '%.16s' directive output may
>> be truncated writing up to 16 bytes into a region of size between 13 and 22 [-Wformat-truncation=]
>>       49 |                  "%d.%d.%04d (%.16s)",
>>          |                               ^~~~~
>>     drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:48:9: note: 'snprintf' output between 12 and
>> 37 bytes into a destination of size 32
>>       48 |         snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       49 |                  "%d.%d.%04d (%.16s)",
>>          |                  ~~~~~~~~~~~~~~~~~~~~~
>>       50 |                  fw_rev_maj(mdev), fw_rev_min(mdev), fw_rev_sub(mdev),
>>          |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       51 |                  mdev->board_id);
>>          |                  ~~~~~~~~~~~~~~~
>> 
>> Fixes: 84e11edb71de ("net/mlx5e: Show board id in ethtool driver information")
>> Link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6d4ab2e97dcfbcd748ae7176
>> 1a9d8e5e41cc732c
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
>> Reviewed-by: Dragos Tatulea <dtatulea@...dia.com>
>> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
>> ---
>>  .../net/ethernet/mellanox/mlx5/core/en_ethtool.c    | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> index 215261a69255..792a0ea544cd 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> @@ -43,12 +43,17 @@ void mlx5e_ethtool_get_drvinfo(struct mlx5e_priv *priv,
>>  			       struct ethtool_drvinfo *drvinfo)
>>  {
>>  	struct mlx5_core_dev *mdev = priv->mdev;
>> +	int count;
>> 
>>  	strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
>> -	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>> -		 "%d.%d.%04d (%.16s)",
>> -		 fw_rev_maj(mdev), fw_rev_min(mdev), fw_rev_sub(mdev),
>> -		 mdev->board_id);
>> +	count = snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>> +			 "%d.%d.%04d (%.16s)", fw_rev_maj(mdev),
>> +			 fw_rev_min(mdev), fw_rev_sub(mdev), mdev->board_id);
>> +	if (count == sizeof(drvinfo->fw_version))

This should be >= now that I think about it.

>From the kernel docs: If the return is greater than or equal to size,
the resulting string is truncated.

The return value *can* be greater than the size parameter expressing
what the length would have been.

https://docs.kernel.org/core-api/kernel-api.html#c.snprintf

>> +		snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>> +			 "%d.%d.%04d", fw_rev_maj(mdev),
>> +			 fw_rev_min(mdev), fw_rev_sub(mdev));
>> +
>>  	strscpy(drvinfo->bus_info, dev_name(mdev->device),
>>  		sizeof(drvinfo->bus_info));
>>  }
>> --
>> 2.41.0
>> 

--
Thanks,

Rahul Rameshbabu