We found a bug in i40e_debugfs.c for the latest linux

We found a bug in i40e_debugfs.c for the latest linux

[Wang Haoran]

Hi, my name is Wang Haoran. We found a bug in the
i40e_dbg_command_read function located in
drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
kernel (version 6.15.5).
The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
together with the network device name (name), a newline character, and
a null terminator, the total formatted string length may exceed the
buffer size of 256 bytes.
Since "snprintf" returns the total number of bytes that would have
been written (the length of  "%s: %s\n" ), this value may exceed the
buffer length passed to copy_to_user(), this will ultimatly cause
function "copy_to_user" report a buffer overflow error.
Replacing snprintf with scnprintf ensures the return value never
exceeds the specified buffer size, preventing such issues.

--- i40e_debugfs.c 2025-07-06 17:04:26.000000000 +0800
+++ i40e_debugfs.c 2025-07-09 15:51:47.259130500 +0800
@@ -70,7 +70,7 @@
  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);

Best regards,
Wang Haoran

Re: We found a bug in i40e_debugfs.c for the latest linux

[Simon Horman]

On Thu, Jul 10, 2025 at 10:14:18AM +0800, Wang Haoran wrote:
> Hi, my name is Wang Haoran. We found a bug in the
> i40e_dbg_command_read function located in
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
> kernel (version 6.15.5).
> The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
> together with the network device name (name), a newline character, and
> a null terminator, the total formatted string length may exceed the
> buffer size of 256 bytes.
> Since "snprintf" returns the total number of bytes that would have
> been written (the length of  "%s: %s\n" ), this value may exceed the
> buffer length passed to copy_to_user(), this will ultimatly cause
> function "copy_to_user" report a buffer overflow error.
> Replacing snprintf with scnprintf ensures the return value never
> exceeds the specified buffer size, preventing such issues.

Thanks Wang Haoran.

I agree that using scnprintf() is a better choice here than snprintf().

But it is not clear to me that this is a bug.

I see that i40e_dbg_command_buf is initialised to be the
empty string. And I don't see it's contents being updated.

While ->name should be no longer than IFNAMSIZ - 1 (=15) bytes long,
excluding the trailing '\0'.

If so, the string formatted by the line below should always
comfortably fit within buf_size (256 bytes).

> 
> --- i40e_debugfs.c 2025-07-06 17:04:26.000000000 +0800
> +++ i40e_debugfs.c 2025-07-09 15:51:47.259130500 +0800
> @@ -70,7 +70,7 @@
>   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);
> 
> Best regards,
> Wang Haoran
> 

Re: We found a bug in i40e_debugfs.c for the latest linux

[Wang Haoran]

Hi Simon,

Thanks for the clarification.

We’ve observed that i40e_dbg_command_buf is
initialized with a fixed size of 256 bytes, but we
didn’t find any assignment statements updating
its contents elsewhere in the kernel source code.

We’re unsure whether this buffer could potentially
be used or modified in other contexts that we
might have missed.

If the buffer is indeed isolated and only used
as currently observed, then the current use of
snprintf() should be safe.

We’d appreciate your confirmation on whether
this buffer could potentially be used beyond its
current scope.

Regards,
Wang Haoran

Simon Horman <horms@kernel.org> 于2025年7月15日周二 02:10写道：
>
> On Thu, Jul 10, 2025 at 10:14:18AM +0800, Wang Haoran wrote:
> > Hi, my name is Wang Haoran. We found a bug in the
> > i40e_dbg_command_read function located in
> > drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
> > kernel (version 6.15.5).
> > The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
> > together with the network device name (name), a newline character, and
> > a null terminator, the total formatted string length may exceed the
> > buffer size of 256 bytes.
> > Since "snprintf" returns the total number of bytes that would have
> > been written (the length of  "%s: %s\n" ), this value may exceed the
> > buffer length passed to copy_to_user(), this will ultimatly cause
> > function "copy_to_user" report a buffer overflow error.
> > Replacing snprintf with scnprintf ensures the return value never
> > exceeds the specified buffer size, preventing such issues.
>
> Thanks Wang Haoran.
>
> I agree that using scnprintf() is a better choice here than snprintf().
>
> But it is not clear to me that this is a bug.
>
> I see that i40e_dbg_command_buf is initialised to be the
> empty string. And I don't see it's contents being updated.
>
> While ->name should be no longer than IFNAMSIZ - 1 (=15) bytes long,
> excluding the trailing '\0'.
>
> If so, the string formatted by the line below should always
> comfortably fit within buf_size (256 bytes).
>
> >
> > --- i40e_debugfs.c 2025-07-06 17:04:26.000000000 +0800
> > +++ i40e_debugfs.c 2025-07-09 15:51:47.259130500 +0800
> > @@ -70,7 +70,7 @@
> >   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);
> >
> > Best regards,
> > Wang Haoran
> >

Re: We found a bug in i40e_debugfs.c for the latest linux

[Simon Horman]

On Tue, Jul 15, 2025 at 09:38:11PM +0800, Wang Haoran wrote:
> Hi Simon,
> 
> Thanks for the clarification.
> 
> We’ve observed that i40e_dbg_command_buf is
> initialized with a fixed size of 256 bytes, but we
> didn’t find any assignment statements updating
> its contents elsewhere in the kernel source code.
> 
> We’re unsure whether this buffer could potentially
> be used or modified in other contexts that we
> might have missed.
> 
> If the buffer is indeed isolated and only used
> as currently observed, then the current use of
> snprintf() should be safe.
> 
> We’d appreciate your confirmation on whether
> this buffer could potentially be used beyond its
> current scope.

Thanks,

My reading is that i40e_dbg_command_buf is declared
as static in i40e_debugfs.c. And thus should only
be updated within the scope of code in that file.

I would be happy to stand corrected on this.

Re: We found a bug in i40e_debugfs.c for the latest linux

[Jacob Keller]

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



On 7/14/2025 11:10 AM, Simon Horman wrote:
> On Thu, Jul 10, 2025 at 10:14:18AM +0800, Wang Haoran wrote:
>> Hi, my name is Wang Haoran. We found a bug in the
>> i40e_dbg_command_read function located in
>> drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
>> kernel (version 6.15.5).
>> The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
>> together with the network device name (name), a newline character, and
>> a null terminator, the total formatted string length may exceed the
>> buffer size of 256 bytes.
>> Since "snprintf" returns the total number of bytes that would have
>> been written (the length of  "%s: %s\n" ), this value may exceed the
>> buffer length passed to copy_to_user(), this will ultimatly cause
>> function "copy_to_user" report a buffer overflow error.
>> Replacing snprintf with scnprintf ensures the return value never
>> exceeds the specified buffer size, preventing such issues.
> 
> Thanks Wang Haoran.
> 
> I agree that using scnprintf() is a better choice here than snprintf().
> 
> But it is not clear to me that this is a bug.
> 
> I see that i40e_dbg_command_buf is initialised to be the
> empty string. And I don't see it's contents being updated.
> 
> While ->name should be no longer than IFNAMSIZ - 1 (=15) bytes long,
> excluding the trailing '\0'.
> 
> If so, the string formatted by the line below should always
> comfortably fit within buf_size (256 bytes).
> 

the string used to be "hello world" back in the day, but that got
removed. I think it was supposed to be some sort of canary to indicate
the driver interface was working. I really don't understand the logic of
these buffers as they're *never* used. (I even checked some of our
out-of-tree releases to see if there was a use there for some reason..
nope.)

We can probably just drop the i40e_dbg_command_buf (and similarly the
i40e_netdev_command_buf) and save ~512K wasted space from the driver
binary. I suppose we could use scnprintf here as well in the off chance
that netdev->name is >256B somehow.

Or possibly we just drop the ability to read from these command files,
since their entire purpose is to enable the debug interface and reading
does nothing except return "<netdev name>: " right now. It doesn't ever
return data, and there are other ways to get the netdev name than
reading from this command file...

>>
>> --- i40e_debugfs.c 2025-07-06 17:04:26.000000000 +0800
>> +++ i40e_debugfs.c 2025-07-09 15:51:47.259130500 +0800
>> @@ -70,7 +70,7 @@
>>   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);
>>
>> Best regards,
>> Wang Haoran
>>
> 


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

Re: We found a bug in i40e_debugfs.c for the latest linux

[Simon Horman]

On Tue, Jul 15, 2025 at 10:12:43AM -0700, Jacob Keller wrote:
> 
> 
> On 7/14/2025 11:10 AM, Simon Horman wrote:
> > On Thu, Jul 10, 2025 at 10:14:18AM +0800, Wang Haoran wrote:
> >> Hi, my name is Wang Haoran. We found a bug in the
> >> i40e_dbg_command_read function located in
> >> drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
> >> kernel (version 6.15.5).
> >> The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
> >> together with the network device name (name), a newline character, and
> >> a null terminator, the total formatted string length may exceed the
> >> buffer size of 256 bytes.
> >> Since "snprintf" returns the total number of bytes that would have
> >> been written (the length of  "%s: %s\n" ), this value may exceed the
> >> buffer length passed to copy_to_user(), this will ultimatly cause
> >> function "copy_to_user" report a buffer overflow error.
> >> Replacing snprintf with scnprintf ensures the return value never
> >> exceeds the specified buffer size, preventing such issues.
> > 
> > Thanks Wang Haoran.
> > 
> > I agree that using scnprintf() is a better choice here than snprintf().
> > 
> > But it is not clear to me that this is a bug.
> > 
> > I see that i40e_dbg_command_buf is initialised to be the
> > empty string. And I don't see it's contents being updated.
> > 
> > While ->name should be no longer than IFNAMSIZ - 1 (=15) bytes long,
> > excluding the trailing '\0'.
> > 
> > If so, the string formatted by the line below should always
> > comfortably fit within buf_size (256 bytes).
> > 
> 
> the string used to be "hello world" back in the day, but that got
> removed. I think it was supposed to be some sort of canary to indicate
> the driver interface was working. I really don't understand the logic of
> these buffers as they're *never* used. (I even checked some of our
> out-of-tree releases to see if there was a use there for some reason..
> nope.)

Thanks for looking into this.  FWIIW, I was also confused about the
intention of the code.

> We can probably just drop the i40e_dbg_command_buf (and similarly the
> i40e_netdev_command_buf) and save ~512K wasted space from the driver
> binary. I suppose we could use scnprintf here as well in the off chance
> that netdev->name is >256B somehow.

I think that using scnprintf() over snprintf() is a good practice.
Even if there is no bug.

I also think saving ~512K is a good idea.

> Or possibly we just drop the ability to read from these command files,
> since their entire purpose is to enable the debug interface and reading
> does nothing except return "<netdev name>: " right now. It doesn't ever
> return data, and there are other ways to get the netdev name than
> reading from this command file...

This seems best to me.  Because we can see that this code, which appears to
have minimal utility, does have some maintenance overhead (i.e. this
thread).

Less is more :)

...

Re: We found a bug in i40e_debugfs.c for the latest linux

[Wang Haoran]

Thanks for the clarification regarding i40e_dbg_command_buf.

Please let me know if you'd like me to submit a patch to
remove this interface, or to replace snprintf() with scnprintf().





Simon Horman <horms@kernel.org> 于2025年7月16日周三 16:37写道：
>
> On Tue, Jul 15, 2025 at 10:12:43AM -0700, Jacob Keller wrote:
> >
> >
> > On 7/14/2025 11:10 AM, Simon Horman wrote:
> > > On Thu, Jul 10, 2025 at 10:14:18AM +0800, Wang Haoran wrote:
> > >> Hi, my name is Wang Haoran. We found a bug in the
> > >> i40e_dbg_command_read function located in
> > >> drivers/net/ethernet/intel/i40e/i40e_debugfs.c in the latest Linux
> > >> kernel (version 6.15.5).
> > >> The buffer "i40e_dbg_command_buf" has a size of 256. When formatted
> > >> together with the network device name (name), a newline character, and
> > >> a null terminator, the total formatted string length may exceed the
> > >> buffer size of 256 bytes.
> > >> Since "snprintf" returns the total number of bytes that would have
> > >> been written (the length of  "%s: %s\n" ), this value may exceed the
> > >> buffer length passed to copy_to_user(), this will ultimatly cause
> > >> function "copy_to_user" report a buffer overflow error.
> > >> Replacing snprintf with scnprintf ensures the return value never
> > >> exceeds the specified buffer size, preventing such issues.
> > >
> > > Thanks Wang Haoran.
> > >
> > > I agree that using scnprintf() is a better choice here than snprintf().
> > >
> > > But it is not clear to me that this is a bug.
> > >
> > > I see that i40e_dbg_command_buf is initialised to be the
> > > empty string. And I don't see it's contents being updated.
> > >
> > > While ->name should be no longer than IFNAMSIZ - 1 (=15) bytes long,
> > > excluding the trailing '\0'.
> > >
> > > If so, the string formatted by the line below should always
> > > comfortably fit within buf_size (256 bytes).
> > >
> >
> > the string used to be "hello world" back in the day, but that got
> > removed. I think it was supposed to be some sort of canary to indicate
> > the driver interface was working. I really don't understand the logic of
> > these buffers as they're *never* used. (I even checked some of our
> > out-of-tree releases to see if there was a use there for some reason..
> > nope.)
>
> Thanks for looking into this.  FWIIW, I was also confused about the
> intention of the code.
>
> > We can probably just drop the i40e_dbg_command_buf (and similarly the
> > i40e_netdev_command_buf) and save ~512K wasted space from the driver
> > binary. I suppose we could use scnprintf here as well in the off chance
> > that netdev->name is >256B somehow.
>
> I think that using scnprintf() over snprintf() is a good practice.
> Even if there is no bug.
>
> I also think saving ~512K is a good idea.
>
> > Or possibly we just drop the ability to read from these command files,
> > since their entire purpose is to enable the debug interface and reading
> > does nothing except return "<netdev name>: " right now. It doesn't ever
> > return data, and there are other ways to get the netdev name than
> > reading from this command file...
>
> This seems best to me.  Because we can see that this code, which appears to
> have minimal utility, does have some maintenance overhead (i.e. this
> thread).
>
> Less is more :)
>
> ...
>
>

Re: We found a bug in i40e_debugfs.c for the latest linux

[Jacob Keller]

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



On 7/16/2025 5:52 AM, Wang Haoran wrote:
> Thanks for the clarification regarding i40e_dbg_command_buf.
> 
> Please let me know if you'd like me to submit a patch to
> remove this interface, or to replace snprintf() with scnprintf().
> 
> 
Since this is a debugfs interface, I think we're safe to drop the read
accesses entirely, without fear of backwards compatibility violations. I
think I can handle making a patch for that, but I'm happy to accept a
patch from you if you want.

It looks like there is some complication as the
i40e_dbg_netdev_ops_write() does appear to use this buffer for scratch
space. I think that would need cleanup to align with how the
i40e_dbg_command_write() function works with an allocated buffer rather
than using this static space in the driver.

Thanks,
Jake


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