Message-ID: <CANZ3JQRwO=4u24Y17cP3byP8mS9VOP5g=sy_Ch_g0xKSDJLhKA@mail.gmail.com>
Date: Wed, 16 Jul 2025 20:52:57 +0800
From: Wang Haoran <haoranwangsec@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: Jacob Keller <jacob.e.keller@...el.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

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@...nel.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 :)
>
> ...
>
>

Powered by blists - more mailing lists