linux-kernel - Re: [PATCH] pipe_fs_i.h: add pipe_buf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKPOu+8F2fZerfaD=-w68p6Hw5EGdjWdzj8F9OYBcQx_ara22w@mail.gmail.com>
Date:   Wed, 20 Sep 2023 00:39:16 +0200
From:   Max Kellermann <max.kellermann@...os.com>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] pipe_fs_i.h: add pipe_buf_init()

On Tue, Sep 19, 2023 at 4:16 PM Christian Brauner <brauner@...nel.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.