pipe_fs_i.h: add pipe_buf_init()

  Adds one central function which shall be used to initialize a newly
allocated struct pipe_buffer.  This shall make the pipe code more
robust for the next time the pipe_buffer struct gets modified, to
avoid leaving new members uninitialized.  Instead, adding new members
should also add a new pipe_buf_init() parameter, which causes
compile-time errors in call sites that were not adapted.

This commit doesn't refactor fs/fuse/dev.c because this code looks
obscure to me; it initializes pipe_buffers incrementally through a
variety of functions, too complicated for me to understand.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 fs/pipe.c                 |  9 +++------
 fs/splice.c               |  9 ++++-----
 include/linux/pipe_fs_i.h | 20 ++++++++++++++++++++
 kernel/watch_queue.c      |  8 +++-----
 mm/filemap.c              |  8 ++------
 mm/shmem.c                |  9 +++------
 6 files changed, 35 insertions(+), 28 deletions(-)
  

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 582f5317ff71..74532e0cb8d7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2850,12 +2850,8 @@ size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
>  		struct pipe_buffer *buf = pipe_head_buf(pipe);
>  		size_t part = min_t(size_t, PAGE_SIZE - offset, size - spliced);
>  
> -		*buf = (struct pipe_buffer) {
> -			.ops	= &page_cache_pipe_buf_ops,
> -			.page	= page,
> -			.offset	= offset,
> -			.len	= part,
> -		};
> +		pipe_buf_init(buf, page, offset, part,
> +			      &page_cache_pipe_buf_ops, 0);
>  		folio_get(folio);
>  		pipe->head++;
>  		page++;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 02e62fccc80d..75d39653b028 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2901,12 +2901,9 @@ static size_t splice_zeropage_into_pipe(struct pipe_inode_info *pipe,
>  	if (!pipe_full(pipe->head, pipe->tail, pipe->max_usage)) {
>  		struct pipe_buffer *buf = pipe_head_buf(pipe);
>  
> -		*buf = (struct pipe_buffer) {
> -			.ops	= &zero_pipe_buf_ops,
> -			.page	= ZERO_PAGE(0),
> -			.offset	= offset,
> -			.len	= size,
> -		};
> +		pipe_buf_init(buf, ZERO_PAGE(0),
> +			      offset, size,
> +			      &zero_pipe_buf_ops, 0);
>  		pipe->head++;
>  	}

So this may cause issues because the compound literal will cause all non
explicitly initialized fields to be initialized to zero values whereas
your new helper wouldn't. So pipe_buf->private may now contain garbage.
Not ideal imho. Does the helper buy us that much overall?
  

On Tue, Sep 19, 2023 at 3:45 PM Christian Brauner <brauner@kernel.org> wrote:
> So pipe_buf->private may now contain garbage.

NULL is just as garbage as the other 2^64-1 possible pointer values.
NULL isn't special here, nobody checks the field for NULL. This field
is specially crafted for exactly one user, and initializing it with
NULL for all others will at best only add unnecessary overhead, and at
worst will hide initialization bugs from sanitiziers who now think
it's indeed properly initialized.

> Does the helper buy us that much overall?

This is about introducing a safer coding style. The lack of a central
initializer function is what caused CVE-2022-0847, which was the worst
security vulnerability in a decade or two.
  

On Tue, Sep 19, 2023 at 03:55:36PM +0200, Max Kellermann wrote:
> On Tue, Sep 19, 2023 at 3:45 PM Christian Brauner <brauner@kernel.org> wrote:
> > So pipe_buf->private may now contain garbage.
> 
> NULL is just as garbage as the other 2^64-1 possible pointer values.
> NULL isn't special here, nobody checks the field for NULL. This field

You're changing how the code currently works which is written in a way
that ensures all fields are initialized to zero. The fact that currently
nothing looks at private is irrelevant.

Following your argument below this might very easily be the cause for
another CVE when something starts looking at this.

Wouldn't it make more sense to have the pipe_buf_init() initialize the
whole thing and for the place where it leaves buf->private untouched you
can just do:

unsigned long private = buf->private
pipe_buf_init(buf, page, 0, 0, &anon_pipe_buf_ops,
	      PIPE_BUF_FLAG_CAN_MERGE, private)

So just use a compound initializer in pipe_buf_init() just like we do in
copy_clone_args_from_user()?
  

Hi--

On 9/19/23 01:07, Max Kellermann wrote:
> Adds one central function which shall be used to initialize a newly
> allocated struct pipe_buffer.  This shall make the pipe code more
> robust for the next time the pipe_buffer struct gets modified, to
> avoid leaving new members uninitialized.  Instead, adding new members
> should also add a new pipe_buf_init() parameter, which causes
> compile-time errors in call sites that were not adapted.
> 
> This commit doesn't refactor fs/fuse/dev.c because this code looks
> obscure to me; it initializes pipe_buffers incrementally through a
> variety of functions, too complicated for me to understand.
> 
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
>  fs/pipe.c                 |  9 +++------
>  fs/splice.c               |  9 ++++-----
>  include/linux/pipe_fs_i.h | 20 ++++++++++++++++++++
>  kernel/watch_queue.c      |  8 +++-----
>  mm/filemap.c              |  8 ++------
>  mm/shmem.c                |  9 +++------
>  6 files changed, 35 insertions(+), 28 deletions(-)
> 

> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 608a9eb86bff..2ef2bb218641 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -176,6 +176,26 @@ static inline struct pipe_buffer *pipe_head_buf(const struct pipe_inode_info *pi
>  	return pipe_buf(pipe, pipe->head);
>  }
>  
> +/**
> + * Initialize a struct pipe_buffer.
> + */

That's not a kernel-doc comment so don't begin it with /**.
Just use /* instead.
Thanks.

> +static inline void pipe_buf_init(struct pipe_buffer *buf,
> +				 struct page *page,
> +				 unsigned int offset, unsigned int len,
> +				 const struct pipe_buf_operations *ops,
> +				 unsigned int flags)
> +{
> +	buf->page = page;
> +	buf->offset = offset;
> +	buf->len = len;
> +	buf->ops = ops;
> +	buf->flags = flags;
> +
> +	/* not initializing the "private" member because it is only
> +	   used by pipe_buf_operations which inject it via struct
> +	   partial_page / struct splice_pipe_desc */
> +}
> +
>  /**
>   * pipe_buf_get - get a reference to a pipe_buffer
>   * @pipe:	the pipe that the buffer belongs to
  

On Tue, Sep 19, 2023 at 4:16 PM Christian Brauner <brauner@kernel.org> wrote:
> You're changing how the code currently works which is written in a way
> that ensures all fields are initialized to zero. The fact that currently
> nothing looks at private is irrelevant.

Two callers were previously using a designated intiializer which
implicitly zero-initializes unmentioned fields; but that was not
intentional, but accidental. It is true that these two call sites are
changed, now omitting the implicit (and unintended) initializer. If
you consider this a problem, I'll re-add it to those two callers. But
adding the "private" initializer to the new function would also change
how the code currently works - fot the other callers.

It's only relevant if the "private" field is part of some API
contract. If that API contract is undocumented, we should add
documentation - what is it? I'll write documentation.

> Following your argument below this might very easily be the cause for
> another CVE when something starts looking at this.

When something starts looking at this, the API contract changes, and
this one function needs to be adjusted.

Without this function, there is not one function, but an arbitrary
number of redundant copies of this initializing code which all need to
be fixed. As I said, only two copies of those currently do initialize
"private", the others do not. Therefore, my patch is strictly an
improvement, and is safer against those alleged future CVEs, which is
the very goal of my patch.

> Wouldn't it make more sense to have the pipe_buf_init() initialize the
> whole thing and for the place where it leaves buf->private untouched you
> can just do:

Would it? I don't think so, but if you insist, I'll add it.
I prefer to leave "private" uninitialized unless there is a reason to
initialize it, for the reason I stated in my previous reply.