On Thu, 2022-10-27 at 12:39 +0200, KP Singh wrote:

On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
[off-list ref] wrote:

quoted

On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:

quoted

On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <
casey@schaufler-ca.com> wrote:

quoted

On 10/25/2022 12:43 AM, Roberto Sassu wrote:

quoted

On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov
wrote:

quoted

I'm looking at security_inode_init_security() and it is
indeed messy.
Per file system initxattrs callback that processes
kmalloc-ed
strings.
Yikes.

In the short term we should denylist inode_init_security
hook to
disallow attaching bpf-lsm there. set/getxattr should be
done
through kfuncs instead of such kmalloc-a-string hack.

Inode_init_security is an example. It could be that the
other hooks are
affected too. What happens if they get arbitrary positive
values too?

TL;DR - Things will go cattywampus.

The LSM infrastructure is an interface that has "grown
organically",
and isn't necessarily consistent in what it requires of the
security
module implementations. There are cases where it assumes that
the
security module hooks are well behaved, as you've discovered.
I have
no small amount of fear that someone is going to provide an
eBPF
program for security_secid_to_secctx(). There has been an
assumption,
oft stated, that all security modules are going to be
reviewed as
part of the upstream process. The review process ought to
catch hooks
that return unacceptable values. Alas, we've lost that with
BPF.

It would take a(nother) major overhaul of the LSM
infrastructure to
make it safe against hooks that are not well behaved. From
what I have
seen so far it wouldn't be easy/convenient/performant to do
it in the
BPF security module either. I personally think that BPF needs
to
ensure that the eBPF implementations don't return
inappropriate values,
but I understand why that is problematic.

That's an accurate statement. Thank you.

Going back to the original question...
We fix bugs when we discover them.
Regardless of the subsystem they belong to.
No finger pointing.

I'm concerned about the following situation:

struct <something> *function()
{

        ret = security_*();
        if (ret)
                return ERR_PTR(ret);

}

int caller()
{
        ptr = function()
        if (IS_ERR(ptr)
                goto out;

        <use of invalid pointer>
}

I quickly found an occurrence of this:

static int lookup_one_common()
{

[...]

        return inode_permission();
}

struct dentry *try_lookup_one_len()
{

[...]

         err = lookup_one_common(&init_user_ns, name, base, len,
&this);
         if (err)
                 return ERR_PTR(err);


Unfortunately, attaching to inode_permission causes the kernel
to crash immediately (it does not happen with negative return
values).

So, I think the fix should be broader, and not limited to the
inode_init_security hook. Will try to see how it can be fixed.

I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
Trivial verifier change.

Thanks, yes this indeed is an issue. We need to do a few things:

1. Restrict some hooks that we know the BPF LSM will never need.
2. A verifier function that checks return values of LSM
hooks.
For most LSK hooks IS_ERR_VALUE is fine, however, there are some
hooks
like *xattr hooks that use a return value of 1 to indicate a
capability check is required which might need special handling.

I looked at security.c:

/*
 * SELinux and Smack integrate the cap call,
 * so assume that all LSMs supplying this call do so.
 */

Other than checking the return value, probably we should also wrap
bpf_lsm_inode_{set,remove}xattr() to do the capability check, right?

Roberto