Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201112015359.1103333-1-lokeshgidra@google.com>
 <20201112015359.1103333-3-lokeshgidra@google.com> <CAHC9VhScpFVtxzU_nUDUc4zGT7+EZKFRpYAm+Ps5vd2AjKkaMQ@mail.gmail.com>
 <CA+EESO7nFwaeSA2xC_FH=O6MtCuORcHPrihwRdt9ecWWLgkBsg@mail.gmail.com>
In-Reply-To: <CA+EESO7nFwaeSA2xC_FH=O6MtCuORcHPrihwRdt9ecWWLgkBsg@mail.gmail.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Wed, 6 Jan 2021 22:08:13 -0500
Message-ID: <CAHC9VhRm-JR2Tt2NQdsVMo2oQZR=KtHkSnJj+GmRvkcF9GqK4w@mail.gmail.com>
Subject: Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface
To:     Lokesh Gidra <lokeshgidra@google.com>
Cc:     Andrea Arcangeli <aarcange@redhat.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        James Morris <jmorris@namei.org>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Casey Schaufler <casey@schaufler-ca.com>,
        Eric Biggers <ebiggers@kernel.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Eric Paris <eparis@parisplace.org>,
        Daniel Colascione <dancol@dancol.org>,
        Kees Cook <keescook@chromium.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        KP Singh <kpsingh@google.com>,
        David Howells <dhowells@redhat.com>,
        Anders Roxell <anders.roxell@linaro.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Matthew Garrett <matthewgarrett@google.com>,
        Aaron Goidel <acgoide@tycho.nsa.gov>,
        Randy Dunlap <rdunlap@infradead.org>,
        "Joel Fernandes (Google)" <joel@joelfernandes.org>,
        YueHaibing <yuehaibing@huawei.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Alexey Budankov <alexey.budankov@linux.intel.com>,
        Adrian Reber <areber@redhat.com>,
        Aleksa Sarai <cyphar@cyphar.com>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        LSM List <linux-security-module@vger.kernel.org>,
        SElinux list <selinux@vger.kernel.org>,
        Kalesh Singh <kaleshsingh@google.com>,
        Calin Juravle <calin@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Jeffrey Vander Stoep <jeffv@google.com>,
        "Cc: Android Kernel" <kernel-team@android.com>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>, hch@infradead.org,
        Daniel Colascione <dancol@google.com>,
        Eric Biggers <ebiggers@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 6, 2021 at 9:44 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> On Wed, Jan 6, 2021 at 6:10 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > From: Daniel Colascione <dancol@google.com>
> > >
> > > This change adds a new function, anon_inode_getfd_secure, that creates
> > > anonymous-node file with individual non-S_PRIVATE inode to which security
> > > modules can apply policy. Existing callers continue using the original
> > > singleton-inode kind of anonymous-inode file. We can transition anonymous
> > > inode users to the new kind of anonymous inode in individual patches for
> > > the sake of bisection and review.
> > >
> > > The new function accepts an optional context_inode parameter that callers
> > > can use to provide additional contextual information to security modules.
> > > For example, in case of userfaultfd, the created inode is a 'logical child'
> > > of the context_inode (userfaultfd inode of the parent process) in the sense
> > > that it provides the security context required during creation of the child
> > > process' userfaultfd inode.
> > >
> > > Signed-off-by: Daniel Colascione <dancol@google.com>
> > >
> > > [Delete obsolete comments to alloc_anon_inode()]
> > > [Add context_inode description in comments to anon_inode_getfd_secure()]
> > > [Remove definition of anon_inode_getfile_secure() as there are no callers]
> > > [Make __anon_inode_getfile() static]
> > > [Use correct error cast in __anon_inode_getfile()]
> > > [Fix error handling in __anon_inode_getfile()]
> >
> > Lokesh, I'm assuming you made the changes in the brackets above?  If
> > so they should include your initials or some other means of
> > attributing them to you, e.g. "[LG: Fix error ...]".
>
> Thanks for reviewing the patch. Sorry for missing this. If it's
> critical then I can upload another version of the patches to fix this.
> Kindly let me know.

Normally that is something I could fix during a merge with your
approval, but see my comments to patch 3/4; I think this patchset
still needs some work.

> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > >  fs/anon_inodes.c            | 150 ++++++++++++++++++++++++++----------
> > >  fs/libfs.c                  |   5 --
> > >  include/linux/anon_inodes.h |   5 ++
> > >  3 files changed, 115 insertions(+), 45 deletions(-)

...

> > > +static struct file *__anon_inode_getfile(const char *name,
> > > +                                        const struct file_operations *fops,
> > > +                                        void *priv, int flags,
> > > +                                        const struct inode *context_inode,
> > > +                                        bool secure)
> >
> > Is it necessary to pass both the context_inode pointer and the secure
> > boolean?  It seems like if context_inode is non-NULL then one could
> > assume that a secure anonymous inode was requested; is there ever
> > going to be a case where this is not true?
>
> Yes, it is necessary as there are scenarios where a secure anon-inode
> is to be created but there is no context_inode available. For
> instance, in patch 4/4 of this series you'll see that when a secure
> anon-inode is created in the userfaultfd syscall, context_inode isn't
> available.

My mistake, I didn't realize this until I got further in the patchset.

-- 
paul moore
www.paul-moore.com