Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Mar 2020 14:44:05 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     KP Singh <kpsingh@chromium.org>, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
        Brendan Jackman <jackmanb@google.com>,
        Florent Revest <revest@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        James Morris <jmorris@namei.org>, Paul Turner <pjt@google.com>,
        Jann Horn <jannh@google.com>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH bpf-next v5 5/7] bpf: lsm: Initialize the BPF LSM hooks
Message-ID: <202003231354.1454ED92EC@keescook>
References: <20200323164415.12943-1-kpsingh@chromium.org>
 <20200323164415.12943-6-kpsingh@chromium.org>
 <202003231237.F654B379@keescook>
 <0655d820-4c42-cf9a-23d3-82dc4fdeeceb@schaufler-ca.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0655d820-4c42-cf9a-23d3-82dc4fdeeceb@schaufler-ca.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 23, 2020 at 01:47:29PM -0700, Casey Schaufler wrote:
> On 3/23/2020 12:44 PM, Kees Cook wrote:
> > On Mon, Mar 23, 2020 at 05:44:13PM +0100, KP Singh wrote:
> >> +/* Some LSM hooks do not have 0 as their default return values. Override the
> >> + * __weak definitons generated by default for these hooks
> > If you wanted to avoid this, couldn't you make the default return value
> > part of lsm_hooks.h?
> >
> > e.g.:
> >
> > LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct inode *inode,
> > 	 const char *name, void **buffer, bool alloc)
> 
> If you're going to do that you'll have to keep lsm_hooks.h and security.c
> default values in sync somehow. Note that the four functions you've called
> out won't be using call_int_hook() after the next round of stacking. I'm not
> nixing the idea, I just don't want the default return for the security_
> functions defined in two places.

Yeah, I actually went looking for this after I sent the email, realizing
that the defaults were also used in security.c. I've been pondering how
to keep them from being duplicated. I'm working on some ideas.

The four are:

inode_getsecurity
inode_setsecurity
task_prctl
xfrm_state_pol_flow_match

None of these are already just calling call_int_hook(), but I assume
they'll need further tweaks in the coming stacking.

To leave things as open-code-able as possible while still benefiting
from the macro consolidation, how about something like this:

lsm_hook_names.h:

LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity,
	 struct inode *inode, const char *name, void **buffer, bool alloc)

...

security.c:

#define LSM_RET_DEFAULT_void(DEFAULT, NAME)	/* */
#define LSM_RET_DEFAULT_int(DEFAULT, NAME)
	static const int NAME#_default = (DEFAULT);

#define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
	LSM_RET_DEFAULT_#RET(DEFAULT, NAME)
#include <linux/lsm_hook_names.h>
#undef LSM_HOOK
...

Then -EOPNOTSUPP is available as "inode_getsecurity_default":

int security_inode_getsecurity(struct inode *inode, const char *name,
			       void **buffer, bool alloc)
{
        struct security_hook_list *hp;
        int rc;

        if (unlikely(IS_PRIVATE(inode)))
                return inode_getsecurity_default;
        /*
         * Only one module will provide an attribute with a given name.
         */
        hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
                rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
                if (rc != inode_getsecurity_default)
                        return rc;
        }
        return inode_getsecurity_default;
}


On the other hand, it's only 4 non-default return codes, so maybe the
sync burden isn't very high?

-- 
Kees Cook