Message-ID: <IA3PR11MB8986590B61415A5A8558BCD5E55FA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Wed, 23 Jul 2025 14:03:21 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: Simon Horman <horms@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, Kunwu Chan <chentao@...inos.cn>, Wang Haoran
	<haoranwangsec@...il.com>, Amir Mohammad Jahangirzad
	<a.jahangirzad@...il.com>, "Keller, Jacob E" <jacob.e.keller@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH] i40e: remove read access to debugfs
 files



> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> Of Jacob Keller
> Sent: Wednesday, July 23, 2025 2:15 AM
> To: Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>;
> netdev@...r.kernel.org
> Cc: Simon Horman <horms@...nel.org>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Kunwu Chan <chentao@...inos.cn>; Wang
> Haoran <haoranwangsec@...il.com>; Amir Mohammad Jahangirzad
> <a.jahangirzad@...il.com>; Keller, Jacob E <jacob.e.keller@...el.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@...inos.cn>
> Closes: https://lore.kernel.org/intel-wired-lan/20231208031950.47410-
> 1-chentao@...inos.cn/
> Reported-by: Wang Haoran <haoranwangsec@...il.com>
> Closes:
> https://lore.kernel.org/all/CANZ3JQRRiOdtfQJoP9QM=6LS1Jto8PGBGw6y7-
> TL=BcnzHQn1Q@...l.gmail.com/
> Reported-by: Amir Mohammad Jahangirzad <a.jahangirzad@...il.com>
> Closes: https://lore.kernel.org/all/20250722115017.206969-1-
> a.jahangirzad@...il.com/
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.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@...el.com>

Powered by blists - more mailing lists