Message-ID: <7cb8dd49-5c8d-1df5-1859-21553ec30637@schaufler-ca.com>
Date:   Fri, 18 Nov 2022 09:15:17 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Roberto Sassu <roberto.sassu@...weicloud.com>,
        Mimi Zohar <zohar@...ux.ibm.com>, dmitry.kasatkin@...il.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        stephen.smalley.work@...il.com, eparis@...isplace.org
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        reiserfs-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
        keescook@...omium.org, nicolas.bouchinet@...p-os.org,
        Roberto Sassu <roberto.sassu@...wei.com>,
        casey@...aufler-ca.com
Subject: Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for
 inode_init_security hook

On 11/18/2022 1:14 AM, Roberto Sassu wrote:
> On 11/17/2022 5:05 PM, Mimi Zohar wrote:
>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@...wei.com>
>>>
>>> Currently, security_inode_init_security() supports only one LSM
>>> providing
>>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>>> metadata.
>>>
>>> Allow all LSMs to provide one or multiple xattrs, by extending the
>>> security
>>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>>> lsm_blob_sizes structure, so that each LSM can specify how many
>>> xattrs it
>>> needs, and the LSM infrastructure knows how many xattr slots it should
>>> allocate.
>>
>> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
>> doesn't currently support it.  The LSM xattrs are hard coded in
>> evm_config_default_xattrnames[],  based on whether the LSM is
>> configured.  Additional security xattrs may be included in the
>> security.evm calculation, by extending the list via
>> security/integrity/evm/evm_xattrs.
>
> EVM wouldn't notice whether it is the same LSM that provide multiple
> xattrs or multiple LSMs provided one xattr. As long as the xattr array
> contains consecutive xattrs, that would be fine. In the IMA/EVM test I
> included a test case where an LSM provides two xattrs (seems to work
> fine).
>
>>> Dynamically allocate the xattrs array to be populated by LSMs with the
>>> inode_init_security hook, and pass it to the latter instead of the
>>> name/value/len triple.
>>>
>>> Since the LSM infrastructure, at initialization time, updates the
>>> number of
>>> the requested xattrs provided by each LSM with a corresponding
>>> offset in
>>> the security blob (in this case the xattr array), it makes
>>> straightforward
>>> for an LSM to access the right position in the xattr array.
>>>
>>> There is still the issue that an LSM might not fill the xattr, even
>>> if it
>>> requests it (legitimate case, for example it might have been loaded
>>> but not
>>> initialized with a policy). Since users of the xattr array (e.g. the
>>> initxattrs() callbacks) detect the end of the xattr array by
>>> checking if
>>> the xattr name is NULL, not filling an xattr would cause those users to
>>> stop scanning xattrs prematurely.
>>>
>>> Solve that issue by introducing security_check_compact_xattrs(),
>>> which does
>>> a basic check of the xattr array (if the xattr name is filled, the
>>> xattr
>>> value should be too, and viceversa), and compacts the xattr array by
>>> removing the holes.
>>>
>>> An alternative solution would be to let users of the xattr array
>>> know the
>>> number of elements of the xattr array, so that they don't have to
>>> check the
>>> termination. However, this seems more invasive, compared to a simple
>>> move
>>> of few array elements.
>>>
>>> Finally, adapt both SELinux and Smack to use the new definition of the
>>> inode_init_security hook, and to correctly fill the designated slots
>>> in the
>>> xattr array.
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
>>> ---
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index a0e9b4ce2341..b62f192de6da 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -30,8 +30,6 @@
>>>   #include <linux/msg.h>
>>>   #include <net/flow.h>
>>>   -#define MAX_LSM_EVM_XATTR    2
>>> -
>>>   /* How many LSMs were built into the kernel? */
>>>   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>>>   @@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct
>>> lsm_blob_sizes *needed)
>>>       lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>>>       lsm_set_blob_size(&needed->lbs_superblock,
>>> &blob_sizes.lbs_superblock);
>>>       lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
>>> +    lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
>>>   }
>>>     /* Prepare LSM for initialization. */
>>> @@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
>>>       init_debug("msg_msg blob size    = %d\n",
>>> blob_sizes.lbs_msg_msg);
>>>       init_debug("superblock blob size = %d\n",
>>> blob_sizes.lbs_superblock);
>>>       init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
>>> +    init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
>>>         /*
>>>        * Create any kmem_caches needed for blobs
>>> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode
>>> *inode, const struct xattr *xattrs,
>>>       return 0;
>>>   }
>>
>>> +static int security_check_compact_xattrs(struct xattr *xattrs,
>>> +                     int num_xattrs, int *checked_xattrs)
>>
>> Perhaps the variable naming is off, making it difficult to read.   So
>> although this is a static function, which normally doesn't require a
>> comment, it's definitely needs one.
>
> Ok, will improve it.
>
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = *checked_xattrs; i < num_xattrs; i++) {
>>
>> If the number of "checked" xattrs was kept up to date, removing the
>> empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
>> to support multiple per LSM xattrs?
>
> An LSM might reserve one or more xattrs, but not set it/them (for
> example because it is not initialized). In this case, removing the
> gaps is needed for all subsequent LSMs.

This is in fact the case with Smack. All file system objects have SMACK64 attributes,
some have SMACK64EXEC, others SMACK64MMAP and still others SMACK64TRANSMUTE.

>
>>> +        if ((!xattrs[i].name && xattrs[i].value) ||
>>> +            (xattrs[i].name && !xattrs[i].value))
>>> +            return -EINVAL;
>>> +
>>> +        if (!xattrs[i].name)
>>> +            continue;
>>> +
>>> +        if (i == *checked_xattrs) {
>>> +            (*checked_xattrs)++;
>>> +            continue;
>>> +        }
>>> +
>>> +        memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
>>> +               sizeof(*xattrs));
>>> +        memset(xattrs + i, 0, sizeof(*xattrs));
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int security_inode_init_security(struct inode *inode, struct inode
>>> *dir,
>>>                    const struct qstr *qstr,
>>>                    const initxattrs initxattrs, void *fs_data)
>>>   {
>>> -    struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>>> -    struct xattr *lsm_xattr, *evm_xattr, *xattr;
>>> -    int ret = -EOPNOTSUPP;
>>> +    struct security_hook_list *P;
>>> +    struct xattr *new_xattrs;
>>> +    struct xattr *xattr;
>>> +    int ret = -EOPNOTSUPP, cur_xattrs = 0;
>>>         if (unlikely(IS_PRIVATE(inode)))
>>>           goto out_exit;
>>>   +    if (!blob_sizes.lbs_xattr)
>>> +        goto out_exit;
>>> +
>>>       if (!initxattrs ||
>>>           (initxattrs == &security_initxattrs && !fs_data)) {
>>>           ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>>> -                    dir, qstr, NULL, NULL, NULL);
>>> +                    dir, qstr, NULL);
>>>           goto out_exit;
>>>       }
>>> -    memset(new_xattrs, 0, sizeof(new_xattrs));
>>> -    lsm_xattr = new_xattrs;
>>> -    ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>>> dir, qstr,
>>> -                        &lsm_xattr->name,
>>> -                        &lsm_xattr->value,
>>> -                        &lsm_xattr->value_len);
>>> -    if (ret)
>>> -        goto out;
>>> +    /* Allocate +1 for EVM and +1 as terminator. */
>>> +    new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2,
>>> sizeof(*new_xattrs),
>>> +                 GFP_NOFS);
>>> +    if (!new_xattrs) {
>>> +        ret = -ENOMEM;
>>> +        goto out_exit;
>>> +    }
>>> +    hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
>>> +                 list) {
>>> +        ret = P->hook.inode_init_security(inode, dir, qstr,
>>> new_xattrs);
>>> +        if (ret && ret != -EOPNOTSUPP)
>>> +            goto out;
>>> +        if (ret == -EOPNOTSUPP)
>>> +            continue;
>>> +        ret = security_check_compact_xattrs(new_xattrs,
>>> +                            blob_sizes.lbs_xattr,
>>> +                            &cur_xattrs);
>>
>> Defining a variable named "cur_xattrs" to indicate the number of xattrs
>> compressed is off.  Perhaps use cur_num_xattrs?   Similarly,
>> "checked_xattrs" should be num_checked_xattrs.  Or change the existing
>> num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.
>
> Ok.
>
> Thanks
>
> Roberto
>
>> thanks,
>>
>> Mimi
>>
>>> +        if (ret < 0) {
>>> +            ret = -ENOMEM;
>>> +            goto out;
>>> +        }
>>> +    }
>>>   -    evm_xattr = lsm_xattr + 1;
>>> -    ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>>> +    ret = evm_inode_init_security(inode, new_xattrs,
>>> +                      new_xattrs + cur_xattrs);
>>>       if (ret)
>>>           goto out;
>>>       ret = initxattrs(inode, new_xattrs, fs_data);
>>> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode
>>> *inode, struct inode *dir,
>>>               continue;
>>>           kfree(xattr->value);
>>>       }
>>> +    kfree(new_xattrs);
>>>   out_exit:
>>>       if (initxattrs == &security_initxattrs)
>>>           return ret;
>

Powered by blists - more mailing lists