[PATCH] i40e: replace snprintf() with scnprintf()

[PATCH] i40e: replace snprintf() with scnprintf()

[Amir Mohammad Jahangirzad]

In i40e_dbg_command_read(), a 256-byte buffer is allocated and filled
using snprintf(), then copied to userspace via copy_to_user().

The issue is that snprintf() returns the number of characters that
*Would* have been written, not the number that actually fit in the buffer.
If the combined length of the netdev name and i40e_dbg_command_buf is
long (e.g. 288 + 3 bytes), snprintf() still returns 291 - even though only
256 bytes were written.

This value is passed to copy_to_user(), which may read past the end of
the buffer and leak kernel memory to userspace.

Replacing snprintf() with scnprintf() fixes this. It returns the actual
number of bytes written, ensuring we only copy valid data.

Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 6cd9da662ae1..19a78052800f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -70,7 +70,7 @@ static ssize_t i40e_dbg_command_read(struct file *filp, char __user *buffer,
 		return -ENOSPC;
 
 	main_vsi = i40e_pf_get_main_vsi(pf);
-	len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
+	len = scnprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
 		       i40e_dbg_command_buf);
 
 	bytes_not_copied = copy_to_user(buffer, buf, len);
-- 
2.43.0

RE: [Intel-wired-lan] [PATCH] i40e: replace snprintf() with scnprintf()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Amir Mohammad Jahangirzad
> Sent: Tuesday, July 22, 2025 1:50 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Amir Mohammad Jahangirzad
> <a.jahangirzad@gmail.com>
> Subject: [Intel-wired-lan] [PATCH] i40e: replace snprintf() with
> scnprintf()
> 
> In i40e_dbg_command_read(), a 256-byte buffer is allocated and filled
> using snprintf(), then copied to userspace via copy_to_user().
> 
> The issue is that snprintf() returns the number of characters that
> *Would* have been written, not the number that actually fit in the
> buffer.
> If the combined length of the netdev name and i40e_dbg_command_buf is
> long (e.g. 288 + 3 bytes), snprintf() still returns 291 - even though
> only
> 256 bytes were written.
> 
> This value is passed to copy_to_user(), which may read past the end of
> the buffer and leak kernel memory to userspace.
> 
> Replacing snprintf() with scnprintf() fixes this. It returns the
> actual number of bytes written, ensuring we only copy valid data.
> 
Can you add 'Fixes:' tag?
And I think this patch should be directed to [Intel-wired-lan] [PATCH iwl-net]
To be backported to all LTS kernels.

> Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 6cd9da662ae1..19a78052800f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -70,7 +70,7 @@ static ssize_t i40e_dbg_command_read(struct file
> *filp, char __user *buffer,
>  		return -ENOSPC;
> 
>  	main_vsi = i40e_pf_get_main_vsi(pf);
> -	len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev-
> >name,
> +	len = scnprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev-
> >name,
>  		       i40e_dbg_command_buf);
> 
>  	bytes_not_copied = copy_to_user(buffer, buf, len);
> --
> 2.43.0

Re: [PATCH] i40e: replace snprintf() with scnprintf()

[Tony Nguyen]

On 7/22/2025 4:50 AM, Amir Mohammad Jahangirzad wrote:
> In i40e_dbg_command_read(), a 256-byte buffer is allocated and filled
> using snprintf(), then copied to userspace via copy_to_user().
> 
> The issue is that snprintf() returns the number of characters that
> *Would* have been written, not the number that actually fit in the buffer.
> If the combined length of the netdev name and i40e_dbg_command_buf is
> long (e.g. 288 + 3 bytes), snprintf() still returns 291 - even though only
> 256 bytes were written.

Hi Amir,

Thank you for the patch. In practice, this won't overflow [1]. However, 
this code can be improved. If you follow the thread, there's 
conversation of the changes that will be made.

Thanks,
Tony

[1] https://lore.kernel.org/netdev/20250714181032.GS721198@horms.kernel.org/

> This value is passed to copy_to_user(), which may read past the end of
> the buffer and leak kernel memory to userspace.
> 
> Replacing snprintf() with scnprintf() fixes this. It returns the actual
> number of bytes written, ensuring we only copy valid data.
> 
> Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 6cd9da662ae1..19a78052800f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -70,7 +70,7 @@ static ssize_t i40e_dbg_command_read(struct file *filp, char __user *buffer,
>   		return -ENOSPC;
>   
>   	main_vsi = i40e_pf_get_main_vsi(pf);
> -	len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
> +	len = scnprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
>   		       i40e_dbg_command_buf);
>   
>   	bytes_not_copied = copy_to_user(buffer, buf, len);

Re: [Intel-wired-lan] [PATCH] i40e: replace snprintf() with scnprintf()

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1012 bytes --]



On 7/22/2025 1:17 PM, Tony Nguyen wrote:
> 
> 
> On 7/22/2025 4:50 AM, Amir Mohammad Jahangirzad wrote:
>> In i40e_dbg_command_read(), a 256-byte buffer is allocated and filled
>> using snprintf(), then copied to userspace via copy_to_user().
>>
>> The issue is that snprintf() returns the number of characters that
>> *Would* have been written, not the number that actually fit in the buffer.
>> If the combined length of the netdev name and i40e_dbg_command_buf is
>> long (e.g. 288 + 3 bytes), snprintf() still returns 291 - even though only
>> 256 bytes were written.
> 
> Hi Amir,
> 
> Thank you for the patch. In practice, this won't overflow [1]. However, 
> this code can be improved. If you follow the thread, there's 
> conversation of the changes that will be made.
> 
> Thanks,
> Tony
> 
> [1] https://lore.kernel.org/netdev/20250714181032.GS721198@horms.kernel.org/
> 

Yep. I have patches to just remove this read interface entirely. I hope
to post them later today.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]