All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Kees Cook <keescook@chromium.org>
Cc: Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
	Michael Ellerman <mpe@ellerman.id.au>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
Date: Tue, 18 Aug 2020 22:07:25 +0200	[thread overview]
Message-ID: <20200818200725.GA1081@lst.de> (raw)
In-Reply-To: <202008181256.CABD56782@keescook>

On Tue, Aug 18, 2020 at 12:58:07PM -0700, Kees Cook wrote:
> On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
> > On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
> > > On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> > > > default_file_splice_write is the last piece of generic code that uses
> > > > set_fs to make the uaccess routines operate on kernel pointers.  It
> > > > implements a "fallback loop" for splicing from files that do not actually
> > > > provide a proper splice_read method.  The usual file systems and other
> > > > high bandwith instances all provide a ->splice_read, so this just removes
> > > > support for various device drivers and procfs/debugfs files.  If splice
> > > > support for any of those turns out to be important it can be added back
> > > > by switching them to the iter ops and using generic_file_splice_read.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > 
> > > This seems a bit disruptive? I feel like this is going to make fuzzers
> > > really noisy (e.g. trinity likes to splice random stuff out of /sys and
> > > /proc).
> > 
> > Noisy in the sence of triggering the pr_debug or because they can't
> > handle -EINVAL?
> 
> Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
> with that, but it seems weird not to have a fall-back, especially if
> something would like to splice a file out of there. But, I'm not opposed
> to the change, it just seems like it might cause pain down the road.

The problem is that without pretending a buffer is in user space when
it actually isn't, we can't have a generic fallback.  So we'll have to
have specific support - I wrote generic support for seq_file, and
willy did for /proc/sys, but at least the first caused a few problems
and a fair amount of churn, so I'd rather see first if we can get
away without it.

> 
> -- 
> Kees Cook
---end quoted text---

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Kees Cook <keescook@chromium.org>
Cc: linux-arch@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
Date: Tue, 18 Aug 2020 22:07:25 +0200	[thread overview]
Message-ID: <20200818200725.GA1081@lst.de> (raw)
In-Reply-To: <202008181256.CABD56782@keescook>

On Tue, Aug 18, 2020 at 12:58:07PM -0700, Kees Cook wrote:
> On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
> > On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
> > > On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> > > > default_file_splice_write is the last piece of generic code that uses
> > > > set_fs to make the uaccess routines operate on kernel pointers.  It
> > > > implements a "fallback loop" for splicing from files that do not actually
> > > > provide a proper splice_read method.  The usual file systems and other
> > > > high bandwith instances all provide a ->splice_read, so this just removes
> > > > support for various device drivers and procfs/debugfs files.  If splice
> > > > support for any of those turns out to be important it can be added back
> > > > by switching them to the iter ops and using generic_file_splice_read.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > 
> > > This seems a bit disruptive? I feel like this is going to make fuzzers
> > > really noisy (e.g. trinity likes to splice random stuff out of /sys and
> > > /proc).
> > 
> > Noisy in the sence of triggering the pr_debug or because they can't
> > handle -EINVAL?
> 
> Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
> with that, but it seems weird not to have a fall-back, especially if
> something would like to splice a file out of there. But, I'm not opposed
> to the change, it just seems like it might cause pain down the road.

The problem is that without pretending a buffer is in user space when
it actually isn't, we can't have a generic fallback.  So we'll have to
have specific support - I wrote generic support for seq_file, and
willy did for /proc/sys, but at least the first caused a few problems
and a fair amount of churn, so I'd rather see first if we can get
away without it.

> 
> -- 
> Kees Cook
---end quoted text---

  reply	other threads:[~2020-08-18 20:07 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-17  7:32 remove the last set_fs() in common code, and remove it for x86 and powerpc Christoph Hellwig
2020-08-17  7:32 ` Christoph Hellwig
2020-08-17  7:32 ` [PATCH 01/11] mem: remove duplicate ops for /dev/zero and /dev/null Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:33   ` Kees Cook
2020-08-18 19:33     ` Kees Cook
2020-08-17  7:32 ` [PATCH 02/11] fs: don't allow kernel reads and writes without iter ops Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:34   ` Kees Cook
2020-08-18 19:34     ` Kees Cook
2020-08-17  7:32 ` [PATCH 03/11] fs: don't allow splice read/write without explicit ops Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:39   ` Kees Cook
2020-08-18 19:39     ` Kees Cook
2020-08-18 19:54     ` Christoph Hellwig
2020-08-18 19:54       ` Christoph Hellwig
2020-08-18 19:58       ` Kees Cook
2020-08-18 19:58         ` Kees Cook
2020-08-18 20:07         ` Christoph Hellwig [this message]
2020-08-18 20:07           ` Christoph Hellwig
2020-08-17  7:32 ` [PATCH 04/11] uaccess: add infrastructure for kernel builds with set_fs() Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:40   ` Kees Cook
2020-08-18 19:40     ` Kees Cook
2020-08-17  7:32 ` [PATCH 05/11] test_bitmap: skip user bitmap tests for !CONFIG_SET_FS Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-17  7:50   ` Christophe Leroy
2020-08-17  7:52     ` Christoph Hellwig
2020-08-17  7:52       ` Christoph Hellwig
2020-08-18 19:43   ` Kees Cook
2020-08-18 19:43     ` Kees Cook
2020-08-17  7:32 ` [PATCH 06/11] lkdtm: disable set_fs-based " Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:32   ` Kees Cook
2020-08-18 19:32     ` Kees Cook
2020-08-17  7:32 ` [PATCH 07/11] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32,64}_types.h Christoph Hellwig
2020-08-17  7:32   ` [PATCH 07/11] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32, 64}_types.h Christoph Hellwig
2020-08-18 19:27   ` [PATCH 07/11] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32,64}_types.h Kees Cook
2020-08-18 19:27     ` Kees Cook
2020-08-17  7:32 ` [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-18 19:44   ` Kees Cook
2020-08-18 19:44     ` Kees Cook
2020-08-18 19:55     ` Christoph Hellwig
2020-08-18 19:55       ` Christoph Hellwig
2020-08-18 19:59       ` Kees Cook
2020-08-18 19:59         ` Kees Cook
2020-08-18 20:00         ` Christoph Hellwig
2020-08-18 20:00           ` Christoph Hellwig
2020-08-18 20:08           ` Kees Cook
2020-08-18 20:08             ` Kees Cook
2020-08-17  7:32 ` [PATCH 09/11] x86: remove address space overrides using set_fs() Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-17  8:23   ` David Laight
2020-08-17  8:23     ` David Laight
2020-08-27  9:37     ` 'Christoph Hellwig'
2020-08-27  9:37       ` 'Christoph Hellwig'
2020-08-18 19:46   ` Kees Cook
2020-08-18 19:46     ` Kees Cook
2020-08-17  7:32 ` [PATCH 10/11] powerpc: use non-set_fs based maccess routines Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-17 15:47   ` Christophe Leroy
2020-08-17  7:32 ` [PATCH 11/11] powerpc: remove address space overrides using set_fs() Christoph Hellwig
2020-08-17  7:32   ` Christoph Hellwig
2020-08-17  7:39 ` remove the last set_fs() in common code, and remove it for x86 and powerpc Christoph Hellwig
2020-08-17  7:39   ` Christoph Hellwig
2020-08-18 17:46 ` Christophe Leroy
2020-08-18 18:05   ` Christoph Hellwig
2020-08-18 18:05     ` Christoph Hellwig
2020-08-18 18:23     ` Christophe Leroy
2020-08-18 18:23       ` Christophe Leroy
2020-08-19  7:16       ` Christophe Leroy
2020-08-19  7:22         ` iter and normal ops on /dev/zero & co, was " Christoph Hellwig
2020-08-19  7:22           ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200818200725.GA1081@lst.de \
    --to=hch@lst.de \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=viro@zeniv.linux.org.uk \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.