* Re: [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
@ 2025-07-23 0:16 ` Jacob Keller
2025-07-23 13:26 ` [Intel-wired-lan] " Dawid Osuchowski
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Jacob Keller @ 2025-07-23 0:16 UTC (permalink / raw)
To: Intel Wired LAN, netdev
Cc: Simon Horman, Anthony Nguyen, Kunwu Chan, Wang Haoran,
Amir Mohammad Jahangirzad
[-- Attachment #1.1: Type: text/plain, Size: 100 bytes --]
> [PATCH] i40e: remove read access to debugfs files
I meant to add 'iwl-net' here, apologies :(
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
2025-07-23 0:16 ` Jacob Keller
@ 2025-07-23 13:26 ` Dawid Osuchowski
2025-07-23 14:03 ` Loktionov, Aleksandr
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Dawid Osuchowski @ 2025-07-23 13:26 UTC (permalink / raw)
To: Jacob Keller, Intel Wired LAN, netdev
Cc: Simon Horman, Anthony Nguyen, Kunwu Chan, Wang Haoran,
Amir Mohammad Jahangirzad
On 2025-07-23 2:14 AM, Jacob Keller wrote:
> The 'command' and 'netdev_ops' debugfs files are a legacy debugging
> interface supported by the i40e driver since its early days by commit
> 02e9c290814c ("i40e: debugfs interface").
>
> Both of these debugfs files provide a read handler which is mostly useless,
> and which is implemented with questionable logic. They both use a static
> 256 byte buffer which is initialized to the empty string. In the case of
> the 'command' file this buffer is literally never used and simply wastes
> space. In the case of the 'netdev_ops' file, the last command written is
> saved here.
>
> On read, the files contents are presented as the name of the device
> followed by a colon and then the contents of their respective static
> buffer. For 'command' this will always be "<device>: ". For 'netdev_ops',
> this will be "<device>: <last command written>". But note the buffer is
> shared between all devices operated by this module. At best, it is mostly
> meaningless information, and at worse it could be accessed simultaneously
> as there doesn't appear to be any locking mechanism.
>
> We have also recently received multiple reports for both read functions
> about their use of snprintf and potential overflow that could result in
> reading arbitrary kernel memory. For the 'command' file, this is definitely
> impossible, since the static buffer is always zero and never written to.
> For the 'netdev_ops' file, it does appear to be possible, if the user
> carefully crafts the command input, it will be copied into the buffer,
> which could be large enough to cause snprintf to truncate, which then
> causes the copy_to_user to read beyond the length of the buffer allocated
> by kzalloc.
>
> A minimal fix would be to replace snprintf() with scnprintf() which would
> cap the return to the number of bytes written, preventing an overflow. A
> more involved fix would be to drop the mostly useless static buffers,
> saving 512 bytes and modifying the read functions to stop needing those as
> input.
>
> Instead, lets just completely drop the read access to these files. These
> are debug interfaces exposed as part of debugfs, and I don't believe that
> dropping read access will break any script, as the provided output is
> pretty useless. You can find the netdev name through other more standard
> interfaces, and the 'netdev_ops' interface can easily result in garbage if
> you issue simultaneous writes to multiple devices at once.
>
> In order to properly remove the i40e_dbg_netdev_ops_buf, we need to
> refactor its write function to avoid using the static buffer. Instead, use
> the same logic as the i40e_dbg_command_write, with an allocated buffer.
> Update the code to use this instead of the static buffer, and ensure we
> free the buffer on exit. This fixes simultaneous writes to 'netdev_ops' on
> multiple devices, and allows us to remove the now unused static buffer
> along with removing the read access.
>
> Reported-by: Kunwu Chan <chentao@kylinos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-chentao@kylinos.cn/
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Closes: https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-TL=BcnzHQn1Q@mail.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-a.jahangirzad@gmail.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
Thanks,
Dawid
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
2025-07-23 0:16 ` Jacob Keller
2025-07-23 13:26 ` [Intel-wired-lan] " Dawid Osuchowski
@ 2025-07-23 14:03 ` Loktionov, Aleksandr
2025-07-23 19:50 ` Simon Horman
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Loktionov, Aleksandr @ 2025-07-23 14:03 UTC (permalink / raw)
To: Keller, Jacob E, Intel Wired LAN, netdev@vger.kernel.org
Cc: Simon Horman, Nguyen, Anthony L, Kunwu Chan, Wang Haoran,
Amir Mohammad Jahangirzad, Keller, Jacob E
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Jacob Keller
> Sent: Wednesday, July 23, 2025 2:15 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>;
> netdev@vger.kernel.org
> Cc: Simon Horman <horms@kernel.org>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Kunwu Chan <chentao@kylinos.cn>; Wang
> Haoran <haoranwangsec@gmail.com>; Amir Mohammad Jahangirzad
> <a.jahangirzad@gmail.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs
> files
>
> The 'command' and 'netdev_ops' debugfs files are a legacy debugging
> interface supported by the i40e driver since its early days by commit
> 02e9c290814c ("i40e: debugfs interface").
>
> Both of these debugfs files provide a read handler which is mostly
> useless, and which is implemented with questionable logic. They both
> use a static
> 256 byte buffer which is initialized to the empty string. In the case
> of the 'command' file this buffer is literally never used and simply
> wastes space. In the case of the 'netdev_ops' file, the last command
> written is saved here.
>
> On read, the files contents are presented as the name of the device
> followed by a colon and then the contents of their respective static
> buffer. For 'command' this will always be "<device>: ". For
> 'netdev_ops', this will be "<device>: <last command written>". But
> note the buffer is shared between all devices operated by this module.
> At best, it is mostly meaningless information, and at worse it could
> be accessed simultaneously as there doesn't appear to be any locking
> mechanism.
>
> We have also recently received multiple reports for both read
> functions about their use of snprintf and potential overflow that
> could result in reading arbitrary kernel memory. For the 'command'
> file, this is definitely impossible, since the static buffer is always
> zero and never written to.
> For the 'netdev_ops' file, it does appear to be possible, if the user
> carefully crafts the command input, it will be copied into the buffer,
> which could be large enough to cause snprintf to truncate, which then
> causes the copy_to_user to read beyond the length of the buffer
> allocated by kzalloc.
>
> A minimal fix would be to replace snprintf() with scnprintf() which
> would cap the return to the number of bytes written, preventing an
> overflow. A more involved fix would be to drop the mostly useless
> static buffers, saving 512 bytes and modifying the read functions to
> stop needing those as input.
>
> Instead, lets just completely drop the read access to these files.
> These are debug interfaces exposed as part of debugfs, and I don't
> believe that dropping read access will break any script, as the
> provided output is pretty useless. You can find the netdev name
> through other more standard interfaces, and the 'netdev_ops' interface
> can easily result in garbage if you issue simultaneous writes to
> multiple devices at once.
>
> In order to properly remove the i40e_dbg_netdev_ops_buf, we need to
> refactor its write function to avoid using the static buffer. Instead,
> use the same logic as the i40e_dbg_command_write, with an allocated
> buffer.
> Update the code to use this instead of the static buffer, and ensure
> we free the buffer on exit. This fixes simultaneous writes to
> 'netdev_ops' on multiple devices, and allows us to remove the now
> unused static buffer along with removing the read access.
>
> Reported-by: Kunwu Chan <chentao@kylinos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-
> 1-chentao@kylinos.cn/
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Closes:
> https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-
> TL=BcnzHQn1Q@mail.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-
> a.jahangirzad@gmail.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> I found several reports of the issues with these read functions going
> at least as far back as 2023, with suggestions to remove the read
> access even back then. None of the fixes got accepted or applied, but
> neither did Intel follow up with removing the interfaces. Its time to
> just drop the read access altogether.
> ---
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 123 ++++------------
> ---------
> 1 file changed, 19 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 6cd9da662ae1..a5c794371dfe 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -40,48 +40,6 @@ static struct i40e_vsi *i40e_dbg_find_vsi(struct
> i40e_pf *pf, int seid)
> * setup, adding or removing filters, or other things. Many of
> * these will be useful for some forms of unit testing.
> **************************************************************/
> -static char i40e_dbg_command_buf[256] = "";
> -
> -/**
> - * i40e_dbg_command_read - read for command datum
> - * @filp: the opened file
> - * @buffer: where to write the data for the user to read
> - * @count: the size of the user's buffer
> - * @ppos: file position offset
> - **/
> -static ssize_t i40e_dbg_command_read(struct file *filp, char __user
> *buffer,
> - size_t count, loff_t *ppos)
> -{
> - struct i40e_pf *pf = filp->private_data;
> - struct i40e_vsi *main_vsi;
> - int bytes_not_copied;
> - int buf_size = 256;
> - char *buf;
> - int len;
> -
> - /* don't allow partial reads */
> - if (*ppos != 0)
> - return 0;
> - if (count < buf_size)
> - return -ENOSPC;
> -
> - buf = kzalloc(buf_size, GFP_KERNEL);
> - if (!buf)
> - return -ENOSPC;
> -
> - main_vsi = i40e_pf_get_main_vsi(pf);
> - len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev-
> >name,
> - i40e_dbg_command_buf);
> -
> - bytes_not_copied = copy_to_user(buffer, buf, len);
> - kfree(buf);
> -
> - if (bytes_not_copied)
> - return -EFAULT;
> -
> - *ppos = len;
> - return len;
> -}
>
> static char *i40e_filter_state_string[] = {
> "INVALID",
> @@ -1621,7 +1579,6 @@ static ssize_t i40e_dbg_command_write(struct
> file *filp, static const struct file_operations i40e_dbg_command_fops
> = {
> .owner = THIS_MODULE,
> .open = simple_open,
> - .read = i40e_dbg_command_read,
> .write = i40e_dbg_command_write,
> };
>
> @@ -1630,48 +1587,6 @@ static const struct file_operations
> i40e_dbg_command_fops = {
> * The netdev_ops entry in debugfs is for giving the driver commands
> * to be executed from the netdev operations.
> **************************************************************/
> -static char i40e_dbg_netdev_ops_buf[256] = "";
> -
> -/**
> - * i40e_dbg_netdev_ops_read - read for netdev_ops datum
> - * @filp: the opened file
> - * @buffer: where to write the data for the user to read
> - * @count: the size of the user's buffer
> - * @ppos: file position offset
> - **/
> -static ssize_t i40e_dbg_netdev_ops_read(struct file *filp, char
> __user *buffer,
> - size_t count, loff_t *ppos)
> -{
> - struct i40e_pf *pf = filp->private_data;
> - struct i40e_vsi *main_vsi;
> - int bytes_not_copied;
> - int buf_size = 256;
> - char *buf;
> - int len;
> -
> - /* don't allow partal reads */
> - if (*ppos != 0)
> - return 0;
> - if (count < buf_size)
> - return -ENOSPC;
> -
> - buf = kzalloc(buf_size, GFP_KERNEL);
> - if (!buf)
> - return -ENOSPC;
> -
> - main_vsi = i40e_pf_get_main_vsi(pf);
> - len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev-
> >name,
> - i40e_dbg_netdev_ops_buf);
> -
> - bytes_not_copied = copy_to_user(buffer, buf, len);
> - kfree(buf);
> -
> - if (bytes_not_copied)
> - return -EFAULT;
> -
> - *ppos = len;
> - return len;
> -}
>
> /**
> * i40e_dbg_netdev_ops_write - write into netdev_ops datum @@ -
> 1685,35 +1600,36 @@ static ssize_t i40e_dbg_netdev_ops_write(struct
> file *filp,
> size_t count, loff_t *ppos)
> {
> struct i40e_pf *pf = filp->private_data;
> + char *cmd_buf, *buf_tmp;
> int bytes_not_copied;
> struct i40e_vsi *vsi;
> - char *buf_tmp;
> int vsi_seid;
> int i, cnt;
>
> /* don't allow partial writes */
> if (*ppos != 0)
> return 0;
> - if (count >= sizeof(i40e_dbg_netdev_ops_buf))
> - return -ENOSPC;
>
> - memset(i40e_dbg_netdev_ops_buf, 0,
> sizeof(i40e_dbg_netdev_ops_buf));
> - bytes_not_copied = copy_from_user(i40e_dbg_netdev_ops_buf,
> - buffer, count);
> - if (bytes_not_copied)
> + cmd_buf = kzalloc(count + 1, GFP_KERNEL);
> + if (!cmd_buf)
> + return count;
> + bytes_not_copied = copy_from_user(cmd_buf, buffer, count);
> + if (bytes_not_copied) {
> + kfree(cmd_buf);
> return -EFAULT;
> - i40e_dbg_netdev_ops_buf[count] = '\0';
> + }
> + cmd_buf[count] = '\0';
>
> - buf_tmp = strchr(i40e_dbg_netdev_ops_buf, '\n');
> + buf_tmp = strchr(cmd_buf, '\n');
> if (buf_tmp) {
> *buf_tmp = '\0';
> - count = buf_tmp - i40e_dbg_netdev_ops_buf + 1;
> + count = buf_tmp - cmd_buf + 1;
> }
>
> - if (strncmp(i40e_dbg_netdev_ops_buf, "change_mtu", 10) == 0) {
> + if (strncmp(cmd_buf, "change_mtu", 10) == 0) {
> int mtu;
>
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[11], "%i %i",
> + cnt = sscanf(&cmd_buf[11], "%i %i",
> &vsi_seid, &mtu);
> if (cnt != 2) {
> dev_info(&pf->pdev->dev, "change_mtu <vsi_seid>
> <mtu>\n"); @@ -1735,8 +1651,8 @@ static ssize_t
> i40e_dbg_netdev_ops_write(struct file *filp,
> dev_info(&pf->pdev->dev, "Could not acquire RTNL
> - please try again\n");
> }
>
> - } else if (strncmp(i40e_dbg_netdev_ops_buf, "set_rx_mode", 11)
> == 0) {
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[11], "%i",
> &vsi_seid);
> + } else if (strncmp(cmd_buf, "set_rx_mode", 11) == 0) {
> + cnt = sscanf(&cmd_buf[11], "%i", &vsi_seid);
> if (cnt != 1) {
> dev_info(&pf->pdev->dev, "set_rx_mode
> <vsi_seid>\n");
> goto netdev_ops_write_done;
> @@ -1756,8 +1672,8 @@ static ssize_t i40e_dbg_netdev_ops_write(struct
> file *filp,
> dev_info(&pf->pdev->dev, "Could not acquire RTNL
> - please try again\n");
> }
>
> - } else if (strncmp(i40e_dbg_netdev_ops_buf, "napi", 4) == 0) {
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[4], "%i",
> &vsi_seid);
> + } else if (strncmp(cmd_buf, "napi", 4) == 0) {
> + cnt = sscanf(&cmd_buf[4], "%i", &vsi_seid);
> if (cnt != 1) {
> dev_info(&pf->pdev->dev, "napi <vsi_seid>\n");
> goto netdev_ops_write_done;
> @@ -1775,21 +1691,20 @@ static ssize_t
> i40e_dbg_netdev_ops_write(struct file *filp,
> dev_info(&pf->pdev->dev, "napi called\n");
> }
> } else {
> - dev_info(&pf->pdev->dev, "unknown command '%s'\n",
> - i40e_dbg_netdev_ops_buf);
> + dev_info(&pf->pdev->dev, "unknown command '%s'\n",
> cmd_buf);
> dev_info(&pf->pdev->dev, "available commands\n");
> dev_info(&pf->pdev->dev, " change_mtu <vsi_seid>
> <mtu>\n");
> dev_info(&pf->pdev->dev, " set_rx_mode <vsi_seid>\n");
> dev_info(&pf->pdev->dev, " napi <vsi_seid>\n");
> }
> netdev_ops_write_done:
> + kfree(cmd_buf);
> return count;
> }
>
> static const struct file_operations i40e_dbg_netdev_ops_fops = {
> .owner = THIS_MODULE,
> .open = simple_open,
> - .read = i40e_dbg_netdev_ops_read,
> .write = i40e_dbg_netdev_ops_write,
> };
>
>
> ---
> base-commit: cf074eca0065bc5142e6004ae236bb35a2687fdf
> change-id: 20250722-jk-drop-debugfs-read-access-994721875248
>
> Best regards,
> --
> Jacob Keller <jacob.e.keller@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
` (2 preceding siblings ...)
2025-07-23 14:03 ` Loktionov, Aleksandr
@ 2025-07-23 19:50 ` Simon Horman
2025-07-25 11:32 ` Kunwu Chan
2025-08-08 6:52 ` Rinitha, SX
5 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2025-07-23 19:50 UTC (permalink / raw)
To: Jacob Keller
Cc: Intel Wired LAN, netdev, Anthony Nguyen, Kunwu Chan, Wang Haoran,
Amir Mohammad Jahangirzad
On Tue, Jul 22, 2025 at 05:14:37PM -0700, Jacob Keller wrote:
> The 'command' and 'netdev_ops' debugfs files are a legacy debugging
> interface supported by the i40e driver since its early days by commit
> 02e9c290814c ("i40e: debugfs interface").
>
> Both of these debugfs files provide a read handler which is mostly useless,
> and which is implemented with questionable logic. They both use a static
> 256 byte buffer which is initialized to the empty string. In the case of
> the 'command' file this buffer is literally never used and simply wastes
> space. In the case of the 'netdev_ops' file, the last command written is
> saved here.
>
> On read, the files contents are presented as the name of the device
> followed by a colon and then the contents of their respective static
> buffer. For 'command' this will always be "<device>: ". For 'netdev_ops',
> this will be "<device>: <last command written>". But note the buffer is
> shared between all devices operated by this module. At best, it is mostly
> meaningless information, and at worse it could be accessed simultaneously
> as there doesn't appear to be any locking mechanism.
>
> We have also recently received multiple reports for both read functions
> about their use of snprintf and potential overflow that could result in
> reading arbitrary kernel memory. For the 'command' file, this is definitely
> impossible, since the static buffer is always zero and never written to.
> For the 'netdev_ops' file, it does appear to be possible, if the user
> carefully crafts the command input, it will be copied into the buffer,
> which could be large enough to cause snprintf to truncate, which then
> causes the copy_to_user to read beyond the length of the buffer allocated
> by kzalloc.
>
> A minimal fix would be to replace snprintf() with scnprintf() which would
> cap the return to the number of bytes written, preventing an overflow. A
> more involved fix would be to drop the mostly useless static buffers,
> saving 512 bytes and modifying the read functions to stop needing those as
> input.
>
> Instead, lets just completely drop the read access to these files. These
> are debug interfaces exposed as part of debugfs, and I don't believe that
> dropping read access will break any script, as the provided output is
> pretty useless. You can find the netdev name through other more standard
> interfaces, and the 'netdev_ops' interface can easily result in garbage if
> you issue simultaneous writes to multiple devices at once.
>
> In order to properly remove the i40e_dbg_netdev_ops_buf, we need to
> refactor its write function to avoid using the static buffer. Instead, use
> the same logic as the i40e_dbg_command_write, with an allocated buffer.
> Update the code to use this instead of the static buffer, and ensure we
> free the buffer on exit. This fixes simultaneous writes to 'netdev_ops' on
> multiple devices, and allows us to remove the now unused static buffer
> along with removing the read access.
>
> Reported-by: Kunwu Chan <chentao@kylinos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-chentao@kylinos.cn/
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Closes: https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-TL=BcnzHQn1Q@mail.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-a.jahangirzad@gmail.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> I found several reports of the issues with these read functions going at
> least as far back as 2023, with suggestions to remove the read access even
> back then. None of the fixes got accepted or applied, but neither did Intel
> follow up with removing the interfaces. Its time to just drop the read
> access altogether.
Thanks for the excellent patch description.
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
` (3 preceding siblings ...)
2025-07-23 19:50 ` Simon Horman
@ 2025-07-25 11:32 ` Kunwu Chan
2025-07-28 9:10 ` [Intel-wired-lan] " Loktionov, Aleksandr
2025-08-08 6:52 ` Rinitha, SX
5 siblings, 1 reply; 8+ messages in thread
From: Kunwu Chan @ 2025-07-25 11:32 UTC (permalink / raw)
To: Jacob Keller, Intel Wired LAN, netdev
Cc: Simon Horman, Anthony Nguyen, Wang Haoran,
Amir Mohammad Jahangirzad
On 2025/7/23 08:14, Jacob Keller wrote:
> The 'command' and 'netdev_ops' debugfs files are a legacy debugging
> interface supported by the i40e driver since its early days by commit
> 02e9c290814c ("i40e: debugfs interface").
>
> Both of these debugfs files provide a read handler which is mostly useless,
> and which is implemented with questionable logic. They both use a static
> 256 byte buffer which is initialized to the empty string. In the case of
> the 'command' file this buffer is literally never used and simply wastes
> space. In the case of the 'netdev_ops' file, the last command written is
> saved here.
>
> On read, the files contents are presented as the name of the device
> followed by a colon and then the contents of their respective static
> buffer. For 'command' this will always be "<device>: ". For 'netdev_ops',
> this will be "<device>: <last command written>". But note the buffer is
> shared between all devices operated by this module. At best, it is mostly
> meaningless information, and at worse it could be accessed simultaneously
> as there doesn't appear to be any locking mechanism.
>
> We have also recently received multiple reports for both read functions
> about their use of snprintf and potential overflow that could result in
> reading arbitrary kernel memory. For the 'command' file, this is definitely
> impossible, since the static buffer is always zero and never written to.
> For the 'netdev_ops' file, it does appear to be possible, if the user
> carefully crafts the command input, it will be copied into the buffer,
> which could be large enough to cause snprintf to truncate, which then
> causes the copy_to_user to read beyond the length of the buffer allocated
> by kzalloc.
>
> A minimal fix would be to replace snprintf() with scnprintf() which would
> cap the return to the number of bytes written, preventing an overflow. A
> more involved fix would be to drop the mostly useless static buffers,
> saving 512 bytes and modifying the read functions to stop needing those as
> input.
>
> Instead, lets just completely drop the read access to these files. These
> are debug interfaces exposed as part of debugfs, and I don't believe that
> dropping read access will break any script, as the provided output is
> pretty useless. You can find the netdev name through other more standard
> interfaces, and the 'netdev_ops' interface can easily result in garbage if
> you issue simultaneous writes to multiple devices at once.
>
> In order to properly remove the i40e_dbg_netdev_ops_buf, we need to
> refactor its write function to avoid using the static buffer. Instead, use
> the same logic as the i40e_dbg_command_write, with an allocated buffer.
> Update the code to use this instead of the static buffer, and ensure we
> free the buffer on exit. This fixes simultaneous writes to 'netdev_ops' on
> multiple devices, and allows us to remove the now unused static buffer
> along with removing the read access.
>
> Reported-by: Kunwu Chan <chentao@kylinos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-chentao@kylinos.cn/
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Closes: https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-TL=BcnzHQn1Q@mail.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-a.jahangirzad@gmail.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> I found several reports of the issues with these read functions going at
> least as far back as 2023, with suggestions to remove the read access even
> back then. None of the fixes got accepted or applied, but neither did Intel
> follow up with removing the interfaces. Its time to just drop the read
> access altogether.
Reviewed-by: Kunwu Chan <kunwu.chan@linux.dev>
> ---
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 123 ++++---------------------
> 1 file changed, 19 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 6cd9da662ae1..a5c794371dfe 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -40,48 +40,6 @@ static struct i40e_vsi *i40e_dbg_find_vsi(struct i40e_pf *pf, int seid)
> * setup, adding or removing filters, or other things. Many of
> * these will be useful for some forms of unit testing.
> **************************************************************/
> -static char i40e_dbg_command_buf[256] = "";
> -
> -/**
> - * i40e_dbg_command_read - read for command datum
> - * @filp: the opened file
> - * @buffer: where to write the data for the user to read
> - * @count: the size of the user's buffer
> - * @ppos: file position offset
> - **/
> -static ssize_t i40e_dbg_command_read(struct file *filp, char __user *buffer,
> - size_t count, loff_t *ppos)
> -{
> - struct i40e_pf *pf = filp->private_data;
> - struct i40e_vsi *main_vsi;
> - int bytes_not_copied;
> - int buf_size = 256;
> - char *buf;
> - int len;
> -
> - /* don't allow partial reads */
> - if (*ppos != 0)
> - return 0;
> - if (count < buf_size)
> - return -ENOSPC;
> -
> - buf = kzalloc(buf_size, GFP_KERNEL);
> - if (!buf)
> - return -ENOSPC;
> -
> - main_vsi = i40e_pf_get_main_vsi(pf);
> - len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
> - i40e_dbg_command_buf);
> -
> - bytes_not_copied = copy_to_user(buffer, buf, len);
> - kfree(buf);
> -
> - if (bytes_not_copied)
> - return -EFAULT;
> -
> - *ppos = len;
> - return len;
> -}
>
> static char *i40e_filter_state_string[] = {
> "INVALID",
> @@ -1621,7 +1579,6 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
> static const struct file_operations i40e_dbg_command_fops = {
> .owner = THIS_MODULE,
> .open = simple_open,
> - .read = i40e_dbg_command_read,
> .write = i40e_dbg_command_write,
> };
>
> @@ -1630,48 +1587,6 @@ static const struct file_operations i40e_dbg_command_fops = {
> * The netdev_ops entry in debugfs is for giving the driver commands
> * to be executed from the netdev operations.
> **************************************************************/
> -static char i40e_dbg_netdev_ops_buf[256] = "";
> -
> -/**
> - * i40e_dbg_netdev_ops_read - read for netdev_ops datum
> - * @filp: the opened file
> - * @buffer: where to write the data for the user to read
> - * @count: the size of the user's buffer
> - * @ppos: file position offset
> - **/
> -static ssize_t i40e_dbg_netdev_ops_read(struct file *filp, char __user *buffer,
> - size_t count, loff_t *ppos)
> -{
> - struct i40e_pf *pf = filp->private_data;
> - struct i40e_vsi *main_vsi;
> - int bytes_not_copied;
> - int buf_size = 256;
> - char *buf;
> - int len;
> -
> - /* don't allow partal reads */
> - if (*ppos != 0)
> - return 0;
> - if (count < buf_size)
> - return -ENOSPC;
> -
> - buf = kzalloc(buf_size, GFP_KERNEL);
> - if (!buf)
> - return -ENOSPC;
> -
> - main_vsi = i40e_pf_get_main_vsi(pf);
> - len = snprintf(buf, buf_size, "%s: %s\n", main_vsi->netdev->name,
> - i40e_dbg_netdev_ops_buf);
> -
> - bytes_not_copied = copy_to_user(buffer, buf, len);
> - kfree(buf);
> -
> - if (bytes_not_copied)
> - return -EFAULT;
> -
> - *ppos = len;
> - return len;
> -}
>
> /**
> * i40e_dbg_netdev_ops_write - write into netdev_ops datum
> @@ -1685,35 +1600,36 @@ static ssize_t i40e_dbg_netdev_ops_write(struct file *filp,
> size_t count, loff_t *ppos)
> {
> struct i40e_pf *pf = filp->private_data;
> + char *cmd_buf, *buf_tmp;
> int bytes_not_copied;
> struct i40e_vsi *vsi;
> - char *buf_tmp;
> int vsi_seid;
> int i, cnt;
>
> /* don't allow partial writes */
> if (*ppos != 0)
> return 0;
> - if (count >= sizeof(i40e_dbg_netdev_ops_buf))
> - return -ENOSPC;
>
> - memset(i40e_dbg_netdev_ops_buf, 0, sizeof(i40e_dbg_netdev_ops_buf));
> - bytes_not_copied = copy_from_user(i40e_dbg_netdev_ops_buf,
> - buffer, count);
> - if (bytes_not_copied)
> + cmd_buf = kzalloc(count + 1, GFP_KERNEL);
> + if (!cmd_buf)
> + return count;
> + bytes_not_copied = copy_from_user(cmd_buf, buffer, count);
> + if (bytes_not_copied) {
> + kfree(cmd_buf);
> return -EFAULT;
> - i40e_dbg_netdev_ops_buf[count] = '\0';
> + }
> + cmd_buf[count] = '\0';
>
> - buf_tmp = strchr(i40e_dbg_netdev_ops_buf, '\n');
> + buf_tmp = strchr(cmd_buf, '\n');
> if (buf_tmp) {
> *buf_tmp = '\0';
> - count = buf_tmp - i40e_dbg_netdev_ops_buf + 1;
> + count = buf_tmp - cmd_buf + 1;
> }
>
> - if (strncmp(i40e_dbg_netdev_ops_buf, "change_mtu", 10) == 0) {
> + if (strncmp(cmd_buf, "change_mtu", 10) == 0) {
> int mtu;
>
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[11], "%i %i",
> + cnt = sscanf(&cmd_buf[11], "%i %i",
> &vsi_seid, &mtu);
> if (cnt != 2) {
> dev_info(&pf->pdev->dev, "change_mtu <vsi_seid> <mtu>\n");
> @@ -1735,8 +1651,8 @@ static ssize_t i40e_dbg_netdev_ops_write(struct file *filp,
> dev_info(&pf->pdev->dev, "Could not acquire RTNL - please try again\n");
> }
>
> - } else if (strncmp(i40e_dbg_netdev_ops_buf, "set_rx_mode", 11) == 0) {
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[11], "%i", &vsi_seid);
> + } else if (strncmp(cmd_buf, "set_rx_mode", 11) == 0) {
> + cnt = sscanf(&cmd_buf[11], "%i", &vsi_seid);
> if (cnt != 1) {
> dev_info(&pf->pdev->dev, "set_rx_mode <vsi_seid>\n");
> goto netdev_ops_write_done;
> @@ -1756,8 +1672,8 @@ static ssize_t i40e_dbg_netdev_ops_write(struct file *filp,
> dev_info(&pf->pdev->dev, "Could not acquire RTNL - please try again\n");
> }
>
> - } else if (strncmp(i40e_dbg_netdev_ops_buf, "napi", 4) == 0) {
> - cnt = sscanf(&i40e_dbg_netdev_ops_buf[4], "%i", &vsi_seid);
> + } else if (strncmp(cmd_buf, "napi", 4) == 0) {
> + cnt = sscanf(&cmd_buf[4], "%i", &vsi_seid);
> if (cnt != 1) {
> dev_info(&pf->pdev->dev, "napi <vsi_seid>\n");
> goto netdev_ops_write_done;
> @@ -1775,21 +1691,20 @@ static ssize_t i40e_dbg_netdev_ops_write(struct file *filp,
> dev_info(&pf->pdev->dev, "napi called\n");
> }
> } else {
> - dev_info(&pf->pdev->dev, "unknown command '%s'\n",
> - i40e_dbg_netdev_ops_buf);
> + dev_info(&pf->pdev->dev, "unknown command '%s'\n", cmd_buf);
> dev_info(&pf->pdev->dev, "available commands\n");
> dev_info(&pf->pdev->dev, " change_mtu <vsi_seid> <mtu>\n");
> dev_info(&pf->pdev->dev, " set_rx_mode <vsi_seid>\n");
> dev_info(&pf->pdev->dev, " napi <vsi_seid>\n");
> }
> netdev_ops_write_done:
> + kfree(cmd_buf);
> return count;
> }
>
> static const struct file_operations i40e_dbg_netdev_ops_fops = {
> .owner = THIS_MODULE,
> .open = simple_open,
> - .read = i40e_dbg_netdev_ops_read,
> .write = i40e_dbg_netdev_ops_write,
> };
>
>
> ---
> base-commit: cf074eca0065bc5142e6004ae236bb35a2687fdf
> change-id: 20250722-jk-drop-debugfs-read-access-994721875248
>
> Best regards,
> --
> Jacob Keller <jacob.e.keller@intel.com>
>
--
Thanks,
TAO.
---
“Life finds a way.”
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs files
2025-07-25 11:32 ` Kunwu Chan
@ 2025-07-28 9:10 ` Loktionov, Aleksandr
0 siblings, 0 replies; 8+ messages in thread
From: Loktionov, Aleksandr @ 2025-07-28 9:10 UTC (permalink / raw)
To: Kunwu Chan, Keller, Jacob E, Intel Wired LAN,
netdev@vger.kernel.org
Cc: Simon Horman, Nguyen, Anthony L, Wang Haoran,
Amir Mohammad Jahangirzad
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kunwu Chan
> Sent: Friday, July 25, 2025 1:33 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN
> <intel-wired-lan@lists.osuosl.org>; netdev@vger.kernel.org
> Cc: Simon Horman <horms@kernel.org>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Wang Haoran <haoranwangsec@gmail.com>;
> Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Subject: Re: [Intel-wired-lan] [PATCH] i40e: remove read access to
> debugfs files
>
> On 2025/7/23 08:14, Jacob Keller wrote:
> > The 'command' and 'netdev_ops' debugfs files are a legacy debugging
> > interface supported by the i40e driver since its early days by
> commit
> > 02e9c290814c ("i40e: debugfs interface").
> >
> > Both of these debugfs files provide a read handler which is mostly
> > useless, and which is implemented with questionable logic. They both
> > use a static
> > 256 byte buffer which is initialized to the empty string. In the
> case
> > of the 'command' file this buffer is literally never used and simply
> > wastes space. In the case of the 'netdev_ops' file, the last command
> > written is saved here.
> >
> > On read, the files contents are presented as the name of the device
> > followed by a colon and then the contents of their respective static
> > buffer. For 'command' this will always be "<device>: ". For
> > 'netdev_ops', this will be "<device>: <last command written>". But
> > note the buffer is shared between all devices operated by this
> module.
> > At best, it is mostly meaningless information, and at worse it could
> > be accessed simultaneously as there doesn't appear to be any locking
> mechanism.
> >
> > We have also recently received multiple reports for both read
> > functions about their use of snprintf and potential overflow that
> > could result in reading arbitrary kernel memory. For the 'command'
> > file, this is definitely impossible, since the static buffer is
> always zero and never written to.
> > For the 'netdev_ops' file, it does appear to be possible, if the
> user
> > carefully crafts the command input, it will be copied into the
> buffer,
> > which could be large enough to cause snprintf to truncate, which
> then
> > causes the copy_to_user to read beyond the length of the buffer
> > allocated by kzalloc.
> >
> > A minimal fix would be to replace snprintf() with scnprintf() which
> > would cap the return to the number of bytes written, preventing an
> > overflow. A more involved fix would be to drop the mostly useless
> > static buffers, saving 512 bytes and modifying the read functions to
> > stop needing those as input.
> >
> > Instead, lets just completely drop the read access to these files.
> > These are debug interfaces exposed as part of debugfs, and I don't
> > believe that dropping read access will break any script, as the
> > provided output is pretty useless. You can find the netdev name
> > through other more standard interfaces, and the 'netdev_ops'
> interface
> > can easily result in garbage if you issue simultaneous writes to
> multiple devices at once.
> >
> > In order to properly remove the i40e_dbg_netdev_ops_buf, we need to
> > refactor its write function to avoid using the static buffer.
> Instead,
> > use the same logic as the i40e_dbg_command_write, with an allocated
> buffer.
> > Update the code to use this instead of the static buffer, and ensure
> > we free the buffer on exit. This fixes simultaneous writes to
> > 'netdev_ops' on multiple devices, and allows us to remove the now
> > unused static buffer along with removing the read access.
> >
> > Reported-by: Kunwu Chan <chentao@kylinos.cn>
> > Closes:
> > https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-
> chentao
> > @kylinos.cn/
> > Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> > Closes:
> > https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-
> TL=
> > BcnzHQn1Q@mail.gmail.com/
> > Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> > Closes:
> > https://lore.kernel.org/all/20250722115017.206969-1-
> a.jahangirzad@gmai
> > l.com/
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> > I found several reports of the issues with these read functions
> going
> > at least as far back as 2023, with suggestions to remove the read
> > access even back then. None of the fixes got accepted or applied,
> but
> > neither did Intel follow up with removing the interfaces. Its time
> to
> > just drop the read access altogether.
>
> Reviewed-by: Kunwu Chan <kunwu.chan@linux.dev>
>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
...
> > ---
> >
> > ---
> > base-commit: cf074eca0065bc5142e6004ae236bb35a2687fdf
> > change-id: 20250722-jk-drop-debugfs-read-access-994721875248
> >
> > Best regards,
> > --
> > Jacob Keller <jacob.e.keller@intel.com>
> >
> --
> Thanks,
> TAO.
> ---
> “Life finds a way.”
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs files
2025-07-23 0:14 [PATCH] i40e: remove read access to debugfs files Jacob Keller
` (4 preceding siblings ...)
2025-07-25 11:32 ` Kunwu Chan
@ 2025-08-08 6:52 ` Rinitha, SX
5 siblings, 0 replies; 8+ messages in thread
From: Rinitha, SX @ 2025-08-08 6:52 UTC (permalink / raw)
To: Keller, Jacob E, Intel Wired LAN, netdev@vger.kernel.org
Cc: Simon Horman, Nguyen, Anthony L, Kunwu Chan, Wang Haoran,
Amir Mohammad Jahangirzad, Keller, Jacob E
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller
> Sent: 23 July 2025 05:45
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; netdev@vger.kernel.org
> Cc: Simon Horman <horms@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kunwu Chan <chentao@kylinos.cn>; Wang Haoran <haoranwangsec@gmail.com>; Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs files
>
> The 'command' and 'netdev_ops' debugfs files are a legacy debugging interface supported by the i40e driver since its early days by commit 02e9c290814c ("i40e: debugfs interface").
>
> Both of these debugfs files provide a read handler which is mostly useless, and which is implemented with questionable logic. They both use a static
256 byte buffer which is initialized to the empty string. In the case of the 'command' file this buffer is literally never used and simply wastes space. In the case of the 'netdev_ops' file, the last command written is saved here.
>
> On read, the files contents are presented as the name of the device followed by a colon and then the contents of their respective static buffer. For 'command' this will always be "<device>: ". For 'netdev_ops', this will be "<device>: <last command written>". But note the buffer is shared between all devices operated by this module. At best, it is mostly meaningless information, and at worse it could be accessed simultaneously as there doesn't appear to be any locking mechanism.
>
> We have also recently received multiple reports for both read functions about their use of snprintf and potential overflow that could result in reading arbitrary kernel memory. For the 'command' file, this is definitely impossible, since the static buffer is always zero and never written to.
> For the 'netdev_ops' file, it does appear to be possible, if the user carefully crafts the command input, it will be copied into the buffer, which could be large enough to cause snprintf to truncate, which then causes the copy_to_user to read beyond the length of the buffer allocated by kzalloc.
>
> A minimal fix would be to replace snprintf() with scnprintf() which would cap the return to the number of bytes written, preventing an overflow. A more involved fix would be to drop the mostly useless static buffers, saving 512 bytes and modifying the read functions to stop needing those as input.
>
> Instead, lets just completely drop the read access to these files. These are debug interfaces exposed as part of debugfs, and I don't believe that dropping read access will break any script, as the provided output is pretty useless. You can find the netdev name through other more standard interfaces, and the 'netdev_ops' interface can easily result in garbage if you issue simultaneous writes to multiple devices at once.
>
> In order to properly remove the i40e_dbg_netdev_ops_buf, we need to refactor its write function to avoid using the static buffer. Instead, use the same logic as the i40e_dbg_command_write, with an allocated buffer.
> Update the code to use this instead of the static buffer, and ensure we free the buffer on exit. This fixes simultaneous writes to 'netdev_ops' on multiple devices, and allows us to remove the now unused static buffer along with removing the read access.
>
> Reported-by: Kunwu Chan <chentao@kylinos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-chentao@kylinos.cn/
> Reported-by: Wang Haoran <haoranwangsec@gmail.com>
> Closes: https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-TL=BcnzHQn1Q@mail.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-a.jahangirzad@gmail.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> I found several reports of the issues with these read functions going at least as far back as 2023, with suggestions to remove the read access even back then. None of the fixes got accepted or applied, but neither did Intel follow up with removing the interfaces. Its time to just drop the read access altogether.
> ---
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 123 ++++---------------------
> 1 file changed, 19 insertions(+), 104 deletions(-)
>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply [flat|nested] 8+ messages in thread