Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <34357c96-fe58-ffe5-e464-4bded8f119d5@huaweicloud.com>
Date:   Wed, 26 Oct 2022 10:42:03 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [RFC][PATCH] bpf: Check xattr name/value pair from
 bpf_lsm_inode_init_security()
Content-Language: en-US
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Casey Schaufler <casey@schaufler-ca.com>
Cc:     KP Singh <kpsingh@kernel.org>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Paul Moore <paul@paul-moore.com>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>, bpf <bpf@vger.kernel.org>,
        LSM List <linux-security-module@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        nicolas.bouchinet@clip-os.org, Mimi Zohar <zohar@linux.ibm.com>,
        linux-integrity <linux-integrity@vger.kernel.org>
References: <20221021164626.3729012-1-roberto.sassu@huaweicloud.com>
 <CAADnVQJHDboosqTy5LTHJtJaWJCWn9rv09jmd_sMgeV_OVQjGg@mail.gmail.com>
 <d7a17e482b7bbf945c92443b45de73f56afea08a.camel@huaweicloud.com>
 <bb7a5986f3d25706269d0fec9906ea73c174b808.camel@huaweicloud.com>
 <CAADnVQL1a2pPAJqzj6oUwupxxfaW38KQswzppAZeZPzmTFhjMg@mail.gmail.com>
 <98353f6c82d3dabdb0d434d7ecf0bfc5295015ad.camel@huaweicloud.com>
 <ffbdcfbe-0c63-2ced-62e3-a7248b7a24f0@schaufler-ca.com>
 <CAADnVQLAXsZRNytPHG0KhYKar3K+=7bq2KPQG77VFOKbnTPHmg@mail.gmail.com>
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
In-Reply-To: <CAADnVQLAXsZRNytPHG0KhYKar3K+=7bq2KPQG77VFOKbnTPHmg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
>>>> 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.

Roberto