i40e: replace snprintf() with scnprintf()

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

Posted by Amir Mohammad Jahangirzad 2 months, 2 weeks ago

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: [PATCH] i40e: replace snprintf() with scnprintf()

Posted by Tony Nguyen 2 months, 2 weeks ago


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

Posted by Jacob Keller 2 months, 2 weeks ago


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.

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

Posted by Loktionov, Aleksandr 2 months, 2 weeks ago


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