netdev - 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: <db65ea9a-7e23-48b9-a05a-cdd98c084598@intel.com>
Date: Tue, 15 Jul 2025 10:12:43 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>, Wang Haoran <haoranwangsec@...il.com>
CC: <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 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
>>
> 



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)