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

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

Message-ID: <20250716083731.GI721198@horms.kernel.org>
Date: Wed, 16 Jul 2025 09:37:31 +0100
From: Simon Horman <horms@...nel.org>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Wang Haoran <haoranwangsec@...il.com>, anthony.l.nguyen@...el.com,
	przemyslaw.kitszel@...el.com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: We found a bug in i40e_debugfs.c for the latest linux

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

...