Message-ID: <b1ba86f7-f943-4913-8265-2a94f3951a88@huaweicloud.com>
Date: Tue, 23 Jul 2024 15:04:39 +0800
From: Xu Kuohai <xukuohai@...weicloud.com>
To: Paul Moore <paul@...l-moore.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org,
 linux-security-module@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-integrity@...r.kernel.org, selinux@...r.kernel.org,
 Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
 Matt Bobrowski <mattbobrowski@...gle.com>,
 Brendan Jackman <jackmanb@...omium.org>, James Morris <jmorris@...ei.org>,
 "Serge E . Hallyn" <serge@...lyn.com>,
 Khadija Kamran <kamrankhadijadj@...il.com>,
 Casey Schaufler <casey@...aufler-ca.com>,
 Ondrej Mosnacek <omosnace@...hat.com>, Kees Cook <keescook@...omium.org>,
 John Johansen <john.johansen@...onical.com>,
 Lukas Bulwahn <lukas.bulwahn@...il.com>,
 Roberto Sassu <roberto.sassu@...wei.com>,
 Shung-Hsi Yu <shung-hsi.yu@...e.com>, Edward Cree <ecree.xilinx@...il.com>,
 Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>,
 Trond Myklebust <trond.myklebust@...merspace.com>,
 Anna Schumaker <anna@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook
 key_getsecurity

On 7/23/2024 5:35 AM, Paul Moore wrote:
> On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai@...weicloud.com> wrote:
>> On 7/19/2024 10:08 AM, Paul Moore wrote:
>>> On Jul 11, 2024 Xu Kuohai <xukuohai@...weicloud.com> wrote:
>>>>
>>>> To be consistent with most LSM hooks, convert the return value of
>>>> hook key_getsecurity to 0 or a negative error code.
>>>>
>>>> Before:
>>>> - Hook key_getsecurity returns length of value on success or a
>>>>     negative error code on failure.
>>>>
>>>> After:
>>>> - Hook key_getsecurity returns 0 on success or a negative error
>>>>     code on failure. An output parameter @len is introduced to hold
>>>>     the length of value on success.
>>>>
>>>> Signed-off-by: Xu Kuohai <xukuohai@...wei.com>
>>>> ---
>>>>    include/linux/lsm_hook_defs.h |  3 ++-
>>>>    include/linux/security.h      |  6 ++++--
>>>>    security/keys/keyctl.c        | 11 ++++++++---
>>>>    security/security.c           | 26 +++++++++++++++++++++-----
>>>>    security/selinux/hooks.c      | 11 +++++------
>>>>    security/smack/smack_lsm.c    | 21 +++++++++++----------
>>>>    6 files changed, 51 insertions(+), 27 deletions(-)
> 
> ...
> 
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 9dd2ae6cf763..2c161101074d 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
>>>>     * security_key_getsecurity() - Get the key's security label
>>>>     * @key: key
>>>>     * @buffer: security label buffer
>>>> + * @len: the length of @buffer (including terminating NULL) on success
>>>>     *
>>>>     * Get a textual representation of the security context attached to a key for
>>>>     * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
>>>>     * storage for the NUL-terminated string and the caller should free it.
>>>>     *
>>>> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
>>>> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
>>>> - *         there is no security label assigned to the key.
>>>> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
>>>> + *         (and a NULL buffer pointer) if there is no security label assigned
>>>> + *         to the key.
>>>>     */
>>>> -int security_key_getsecurity(struct key *key, char **buffer)
>>>> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
>>>>    {
>>>> +    int rc;
>>>> +    size_t n = 0;
>>>> +    struct security_hook_list *hp;
>>>> +
>>>>       *buffer = NULL;
>>>> -    return call_int_hook(key_getsecurity, key, buffer);
>>>> +
>>>> +    hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
>>>> +            rc = hp->hook.key_getsecurity(key, buffer, &n);
>>>> +            if (rc < 0)
>>>> +                    return rc;
>>>> +            if (n)
>>>> +                    break;
>>>> +    }
>>>> +
>>>> +    *len = n;
>>>> +
>>>> +    return 0;
>>>>    }
>>>
>>> Help me understand why we can't continue to use the call_int_hook()
>>> macro here?
>>>
>>
>> Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
>> breaks the loop when the hook return value is not 0.
>>
>> After this patch, the +ve is stored in @n, so @n and return value should
>> both be checked to determine whether to break the loop. This is not
>> feasible with call_int_hook.
> 
> Yes, gotcha.  I was focused on the error condition and wasn't thinking
> about the length getting zero'd out by a trailing callback.
> Unfortunately, we *really* want to stick with the
> call_{int,void}_hook() macros so I think we either need to find a way
> to work within that constraint for existing macro callers, or we have
> to leave this hook as-is for the moment.
> 

Let's leave it as is. So we ultimately have four hooks that can be
converted, two of which require adding additional output parameter to
hold odd return values. These output parameters require extra work
on the BPF verifier, and it doesn't seem worthwhile just for two
hooks. So I prefer to keep only the two patches that handle
conversion without adding output parameters (patch 1 and 5).

Powered by blists - more mailing lists