| Message ID | 20230919080707.1077426-1-max.kellermann@ionos.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pipe_fs_i.h: add pipe_buf_init() | expand |
> 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.
diff --git a/fs/pipe.c b/fs/pipe.c index 6c1a9b1db907..edba8c666c95 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -520,14 +520,11 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) /* Insert it into the buffer array */ buf = &pipe->bufs[head & mask]; - buf->page = page; - buf->ops = &anon_pipe_buf_ops; - buf->offset = 0; - buf->len = 0; + pipe_buf_init(buf, page, 0, 0, + &anon_pipe_buf_ops, + PIPE_BUF_FLAG_CAN_MERGE); if (is_packetized(filp)) buf->flags = PIPE_BUF_FLAG_PACKET; - else - buf->flags = PIPE_BUF_FLAG_CAN_MERGE; pipe->tmp_page = NULL; copied = copy_page_from_iter(page, 0, PAGE_SIZE, from); diff --git a/fs/splice.c b/fs/splice.c index d983d375ff11..277bc4812164 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -215,12 +215,11 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe, while (!pipe_full(head, tail, pipe->max_usage)) { struct pipe_buffer *buf = &pipe->bufs[head & mask]; - buf->page = spd->pages[page_nr]; - buf->offset = spd->partial[page_nr].offset; - buf->len = spd->partial[page_nr].len; + pipe_buf_init(buf, spd->pages[page_nr], + spd->partial[page_nr].offset, + spd->partial[page_nr].len, + spd->ops, 0); buf->private = spd->partial[page_nr].private; - buf->ops = spd->ops; - buf->flags = 0; head++; pipe->head = head; 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. + */ +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 diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c index d0b6b390ee42..187ad7ca38b0 100644 --- a/kernel/watch_queue.c +++ b/kernel/watch_queue.c @@ -125,12 +125,10 @@ static bool post_one_notification(struct watch_queue *wqueue, kunmap_atomic(p); buf = &pipe->bufs[head & mask]; - buf->page = page; + pipe_buf_init(buf, page, offset, len, + &watch_queue_pipe_buf_ops, + PIPE_BUF_FLAG_WHOLE); buf->private = (unsigned long)wqueue; - buf->ops = &watch_queue_pipe_buf_ops; - buf->offset = offset; - buf->len = len; - buf->flags = PIPE_BUF_FLAG_WHOLE; smp_store_release(&pipe->head, head + 1); /* vs pipe_read() */ if (!test_and_clear_bit(note, wqueue->notes_bitmap)) { 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++; }
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(-)