[PATCH v2 00/11] Avoid unprivileged splice(file->)/(->socket) pipe exclusion

[PATCH v2 00/11] Avoid unprivileged splice(file->)/(->socket) pipe exclusion

[Ahelenia Ziemiańska]

[-- Attachment #1: Type: text/plain, Size: 4391 bytes --]

Hi!

As it stands, splice(file -> pipe):
1. locks the pipe,
2. does a read from the file,
3. unlocks the pipe.

When the file resides on a normal filesystem, this isn't an issue
because the filesystem has been defined as trusted by root having
mounted it.

But when the file is actually IPC (FUSE) or is just IPC (sockets)
or is a tty, this means that the pipe lock will be held for an
attacker-controlled length of time, and during that time every
process trying to read from, write to, open, or close the pipe
enters an uninterruptible sleep, and will only exit it if the
splicing process is killed.

This trivially denies service to:
* any hypothetical pipe-based log collexion system
* all nullmailer installations
* me, personally, when I'm pasting stuff into qemu -serial chardev:pipe

A symmetric situation happens for splicing(pipe -> socket):
the pipe is locked for as long as the socket is full.

This follows:
1. https://lore.kernel.org/linux-fsdevel/qk6hjuam54khlaikf2ssom6custxf5is2ekkaequf4hvode3ls@zgf7j5j4ubvw/t/#u
2. a security@ thread rooted in
   <irrrblivicfc7o3lfq7yjm2lrxq35iyya4gyozlohw24gdzyg7@azmluufpdfvu>
3. https://nabijaczleweli.xyz/content/blogn_t/011-linux-splice-exclusion.html
4. https://lore.kernel.org/lkml/cover.1697486714.git.nabijaczleweli@nabijaczleweli.xyz/t/#u  (v1)
   https://lore.kernel.org/lkml/1cover.1697486714.git.nabijaczleweli@nabijaczleweli.xyz/t/#u (resend)
   https://lore.kernel.org/lkml/2cover.1697486714.git.nabijaczleweli@nabijaczleweli.xyz/t/#u (reresend)
5. https://lore.kernel.org/lkml/dtexwpw6zcdx7dkx3xj5gyjp5syxmyretdcbcdtvrnukd4vvuh@tarta.nabijaczleweli.xyz/t/#u
   (relay_file_splice_read removal)

1-7/11 request MSG_DONTWAIT (sockets)/IOCB_NOWAIT (generic) on the read

  8/11 removes splice_read from tty completely

  9/11 removes splice_read from FUSE filesystems
       (except virtiofs which has normal mounting security semantics,
        but is handled via FUSE code)

 10/11 allows splice_read from FUSE filesystems mounted by real root
       (this matches the blessing received by non-FUSE network filesystems)

 11/11 requests MSG_DONTWAIT for splice(pipe -> socket).

 12/11 has the man-pages patch with draft wording.

All but 5/11 (AF_SMC) have been tested and embed shell programs to
repro them. AIUI I'd need an s390 machine for it? It's trivial.

6/11 (AF_KCM) also fixes kcm_splice_read() passing SPLICE_F_*-style
flags to skb_recv_datagram(), which takes MSG_*-style flags. I don't
think they did anything anyway? But.

There are two implementations that definitely sleep all the time
and I didn't touch them:
  tracing_splice_read_pipe
  tracing_buffers_splice_read (dropped in v2, v1 4/11)
the semantics are lost on me, but they're in debugfs/tracefs, so
it doesn't matter if they block so long as they work, and presumably
they do right now.

There is also relay_file_splice_read (dropped in v2, v1 5/11),
which isn't an implementation at all because it's dead code, broken,
and removed in -mm.

The diffs in 1-7,11/11 are unchanged, save for a rebase in 7/11.
8/11 replaces the file type test in v1 10/11.
9/11 and 10/11 are new in v2.

Ahelenia Ziemiańska (11):
  splice: copy_splice_read: do the I/O with IOCB_NOWAIT
  af_unix: unix_stream_splice_read: always request MSG_DONTWAIT
  fuse: fuse_dev_splice_read: use nonblocking I/O
  net/smc: smc_splice_read: always request MSG_DONTWAIT
  kcm: kcm_splice_read: always request MSG_DONTWAIT
  tls/sw: tls_sw_splice_read: always request non-blocking I/O
  net/tcp: tcp_splice_read: always do non-blocking reads
  tty: splice_read: disable
  fuse: file: limit splice_read to virtiofs
  fuse: allow splicing from filesystems mounted by real root
  splice: splice_to_socket: always request MSG_DONTWAIT

 drivers/tty/tty_io.c |  2 --
 fs/fuse/dev.c        | 10 ++++++----
 fs/fuse/file.c       | 17 ++++++++++++++++-
 fs/fuse/fuse_i.h     |  4 ++++
 fs/fuse/inode.c      |  2 ++
 fs/fuse/virtio_fs.c  |  1 +
 fs/splice.c          |  5 ++---
 net/ipv4/tcp.c       | 32 +++-----------------------------
 net/kcm/kcmsock.c    |  2 +-
 net/smc/af_smc.c     |  6 +-----
 net/tls/tls_sw.c     |  5 ++---
 net/unix/af_unix.c   |  5 +----
 12 files changed, 39 insertions(+), 52 deletions(-)

base-commit: 2cf4f94d8e8646803f8fb0facf134b0cd7fb691a
--
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 08/11] tty: splice_read: disable

[Ahelenia Ziemiańska]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

We request non-blocking I/O in the generic copy_splice_read, but
"the tty layer doesn't actually honor the IOCB_NOWAIT flag for
various historical reasons.". This means that a tty->pipe splice
will happily sleep with the pipe locked forever, and any process
trying to take it (due to an open/read/write/&c.) will enter
uninterruptible sleep.

This also masks inconsistent wake-ups (usually every second line)
when splicing from ttys in icanon mode.

Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 drivers/tty/tty_io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 06414e43e0b5..50c2957a9c7f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -465,7 +465,6 @@ static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= tty_write,
-	.splice_read	= copy_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
@@ -480,7 +479,6 @@ static const struct file_operations console_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= redirected_tty_write,
-	.splice_read	= copy_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
-- 
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 08/11] tty: splice_read: disable

[Greg Kroah-Hartman]

On Thu, Dec 21, 2023 at 04:09:10AM +0100, Ahelenia Ziemiańska wrote:
> We request non-blocking I/O in the generic copy_splice_read, but
> "the tty layer doesn't actually honor the IOCB_NOWAIT flag for
> various historical reasons.". This means that a tty->pipe splice
> will happily sleep with the pipe locked forever, and any process
> trying to take it (due to an open/read/write/&c.) will enter
> uninterruptible sleep.
> 
> This also masks inconsistent wake-ups (usually every second line)
> when splicing from ttys in icanon mode.
> 
> Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 08/11] tty: splice_read: disable

[Jiri Slaby]

On 21. 12. 23, 4:09, Ahelenia Ziemiańska wrote:
> We request non-blocking I/O in the generic copy_splice_read, but
> "the tty layer doesn't actually honor the IOCB_NOWAIT flag for
> various historical reasons.". This means that a tty->pipe splice
> will happily sleep with the pipe locked forever, and any process
> trying to take it (due to an open/read/write/&c.) will enter
> uninterruptible sleep.
> 
> This also masks inconsistent wake-ups (usually every second line)
> when splicing from ttys in icanon mode.
> 
> Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>   drivers/tty/tty_io.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 06414e43e0b5..50c2957a9c7f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -465,7 +465,6 @@ static const struct file_operations tty_fops = {
>   	.llseek		= no_llseek,
>   	.read_iter	= tty_read,
>   	.write_iter	= tty_write,
> -	.splice_read	= copy_splice_read,
>   	.splice_write	= iter_file_splice_write,

This and the other patch effectively reverts dd78b0c483e33 and 
9bb48c82aced0. I.e. it breaks "things". Especially:

commit 9bb48c82aced07698a2d08ee0f1475a6c4f6b266
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jan 19 11:41:16 2021 -0800

     tty: implement write_iter

     This makes the tty layer use the .write_iter() function instead of the
     traditional .write() functionality.

     That allows writev(), but more importantly also makes it possible to
     enable .splice_write() for ttys, reinstating the "splice to tty"
     functionality that was lost in commit 36e2c7421f02 ("fs: don't allow
     splice read/write without explicit ops").

     Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without 
explicit ops")


What are those "things" doing that "splice to tty", I don't recall and 
the commit message above ^^^ does not spell that out. Linus?

thanks,
-- 
js

Re: [PATCH v2 08/11] tty: splice_read: disable

[Linus Torvalds]

On Wed, 3 Jan 2024 at 03:36, Jiri Slaby <jirislaby@kernel.org> wrote:
>
> What are those "things" doing that "splice to tty", I don't recall and
> the commit message above ^^^ does not spell that out. Linus?

It's some annoying SSL VPN thing that splices to pppd:

   https://lore.kernel.org/all/C8KER7U60WXE.25UFD8RE6QZQK@oguc/

and I'd be happy to try to limit splice to tty's to maybe just the one
case that pppd uses.

So I don't think we should remove splice_write for tty's entirely, but
maybe we can limit it to only the case that the VPN thing used.

I never saw the original issue personally, and I think only Oliver
reported it, so cc'ing Oliver.

Maybe that VPN thing already has the pty in non-blocking mode, for
example, and we could make the tty splicing fail for any blocking op?

                Linus

Re: [PATCH v2 08/11] tty: splice_read: disable

[Oliver Giles]

On Wed, Jan 3 2024 at 11:14:59 -08:00:00, Linus Torvalds 
<torvalds@linux-foundation.org> wrote:
> 
> It's some annoying SSL VPN thing that splices to pppd:
> 
>    https://lore.kernel.org/all/C8KER7U60WXE.25UFD8RE6QZQK@oguc/

I'm happy to report that that particular SSL VPN tool is no longer 
around.
And it had anyway grown a fall-back-to-read/write in case splice() 
fails.
So at least from my perspective, no objections to splice-to-tty going 
away
altogether.

> and I'd be happy to try to limit splice to tty's to maybe just the one
> case that pppd uses.

To be exact, pppd is just providing a pty with which other (now all 
extinct?)
applications can do nefarious things.

> Maybe that VPN thing already has the pty in non-blocking mode, for
> example, and we could make the tty splicing fail for any blocking op?

FWIW, the SSL VPN tool did indeed have the pty in non-blocking mode.

Oliver

Re: [PATCH v2 08/11] tty: splice_read: disable

[Linus Torvalds]

On Wed, 3 Jan 2024 at 13:34, Oliver Giles <ohw.giles@gmail.com> wrote:
>
> I'm happy to report that that particular SSL VPN tool is no longer
> around.

Ahh, well that simplifies things and we can then just remove the tty
splice support again.

Of course, maybe then somebody else will report on some other odd
user, but ... fingers crossed.

                Linus

[PATCH v2 13/11] tty: splice_write: disable

[Ahelenia Ziemiańska]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

Given:
	cat > ttyW.c <<^D
	#define _GNU_SOURCE
	#include <fcntl.h>
	#include <stdlib.h>
	int main()
	{
		int pt = posix_openpt(O_RDWR);
		grantpt(pt);
		unlockpt(pt);
		int cl = open(ptsname(pt), O_WRONLY);
		for (;;)
			splice(0, 0, cl, 0, 128 * 1024 * 1024, 0);
	}
	^D
	cc ttyW.c -o ttyW
	mkfifo fifo
	truncate 32M 32M
	./ttyW < fifo &
	cp 32M fifo &
	sleep 0.1
	read -r _ < fifo
ttyW used to sleep in splice and the shell used to enter an
uninterruptible sleep in open("fifo");
now the splice returns -EINVAL and the whole program completes.

This is also symmetric with the splice_read removal.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
It hit me that I should actually probably exhaustively re-evaluate
splice_write as well since re-evaluating splice_read went so well.

fs/fuse/dev.c:  .splice_write   = fuse_dev_splice_write,
drivers/char/virtio_console.c:  .splice_write = port_fops_splice_write,
locks, takes some pages, unlocks, writes, so OK

drivers/char/mem.c:	.splice_write	= splice_write_null,
drivers/char/mem.c:	.splice_write	= splice_write_zero,
no-op

drivers/char/random.c:	.splice_write = iter_file_splice_write,
drivers/char/random.c:	.splice_write = iter_file_splice_write,
AFAICT write_pool_user is okay to invoke like this?

net/socket.c:   .splice_write = splice_to_socket,
already dealt with in 11/11

drivers/tty/tty_io.c:   .splice_write   = iter_file_splice_write,
drivers/tty/tty_io.c:   .splice_write   = iter_file_splice_write,
they do lock the pipe and try the write with the lock held;
we already killed splice_read so just kill splice_write for symmetry
(13/11)

fs/fuse/file.c: .splice_write   = iter_file_splice_write,
same logic as splice_read applies (14/11)

 drivers/tty/tty_io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 50c2957a9c7f..d931c34ddcbf 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -465,7 +465,6 @@ static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= tty_write,
-	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
 	.compat_ioctl	= tty_compat_ioctl,
@@ -479,7 +478,6 @@ static const struct file_operations console_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= redirected_tty_write,
-	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
 	.compat_ioctl	= tty_compat_ioctl,
-- 
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]