Message-ID: <IA3PR11MB898687EC5F1D1583A8678A80E55AA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 28 Jul 2025 09:10:22 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Kunwu Chan <kunwu.chan@...ux.dev>, "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>, Wang Haoran <haoranwangsec@...il.com>, "Amir
 Mohammad Jahangirzad" <a.jahangirzad@...il.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 Kunwu Chan
> Sent: Friday, July 25, 2025 1:33 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>; 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>; Wang Haoran <haoranwangsec@...il.com>;
> Amir Mohammad Jahangirzad <a.jahangirzad@...il.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@...inos.cn>
> > Closes:
> > https://lore.kernel.org/intel-wired-lan/20231208031950.47410-1-
> chentao
> > @kylinos.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@...i
> > l.com/
> > Signed-off-by: Jacob Keller <jacob.e.keller@...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.
> 
> Reviewed-by: Kunwu Chan <kunwu.chan@...ux.dev>
> 
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>

...

> > ---
> >
> > ---
> > base-commit: cf074eca0065bc5142e6004ae236bb35a2687fdf
> > change-id: 20250722-jk-drop-debugfs-read-access-994721875248
> >
> > Best regards,
> > --
> > Jacob Keller <jacob.e.keller@...el.com>
> >
> --
> Thanks,
>         TAO.
> ---
> “Life finds a way.”

Powered by blists - more mailing lists