[v4,3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook
Commit Message
From: Roberto Sassu <roberto.sassu@huawei.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.
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@huawei.com>
---
include/linux/lsm_hook_defs.h | 3 +-
include/linux/lsm_hooks.h | 17 +++++---
security/security.c | 77 +++++++++++++++++++++++++++--------
security/selinux/hooks.c | 19 +++++----
security/smack/smack_lsm.c | 26 +++++++-----
5 files changed, 100 insertions(+), 42 deletions(-)
Comments
hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.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.
>
> 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@huawei.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.
> +{
> + 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?
> + 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.
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;
On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.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.
Smack uses multiple xattrs. All file system objects have a SMACK64
attribute, which is used for access control. A program file may have
a SMACK64EXEC attribute, which is the label the program will run with.
A library may have a SMACK64MMAP attribute to restrict loading. A
directory may have a SMACK64TRANSMUTE attribute, which modifies the
new object creation behavior.
The point being that it may be more than a "nice idea" to support
multiple xattrs. It's not a hypothetical situation.
On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> > hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >> From: Roberto Sassu <roberto.sassu@huawei.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.
>
> Smack uses multiple xattrs. All file system objects have a SMACK64
> attribute, which is used for access control. A program file may have
> a SMACK64EXEC attribute, which is the label the program will run with.
> A library may have a SMACK64MMAP attribute to restrict loading. A
> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> new object creation behavior.
>
> The point being that it may be more than a "nice idea" to support
> multiple xattrs. It's not a hypothetical situation.
And each of these addiitonal Smack xattrs are already defined in
evm_config_default_xattrnames[].
Mimi
On 11/17/2022 9:24 AM, Mimi Zohar wrote:
> On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
>> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
>>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>>> From: Roberto Sassu <roberto.sassu@huawei.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.
>> Smack uses multiple xattrs. All file system objects have a SMACK64
>> attribute, which is used for access control. A program file may have
>> a SMACK64EXEC attribute, which is the label the program will run with.
>> A library may have a SMACK64MMAP attribute to restrict loading. A
>> directory may have a SMACK64TRANSMUTE attribute, which modifies the
>> new object creation behavior.
>>
>> The point being that it may be more than a "nice idea" to support
>> multiple xattrs. It's not a hypothetical situation.
> And each of these addiitonal Smack xattrs are already defined in
> evm_config_default_xattrnames[].
Then I'm confused by the statement that "EVM doesn't currently support it".
On Thu, 2022-11-17 at 09:40 -0800, Casey Schaufler wrote:
> On 11/17/2022 9:24 AM, Mimi Zohar wrote:
> > On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
> >> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> >>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >>>> From: Roberto Sassu <roberto.sassu@huawei.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.
> >> Smack uses multiple xattrs. All file system objects have a SMACK64
> >> attribute, which is used for access control. A program file may have
> >> a SMACK64EXEC attribute, which is the label the program will run with.
> >> A library may have a SMACK64MMAP attribute to restrict loading. A
> >> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> >> new object creation behavior.
> >>
> >> The point being that it may be more than a "nice idea" to support
> >> multiple xattrs. It's not a hypothetical situation.
> > And each of these addiitonal Smack xattrs are already defined in
> > evm_config_default_xattrnames[].
>
> Then I'm confused by the statement that "EVM doesn't currently support it".
My mistake. As you pointed out, Smack is defining multiple security
xattrs.
Mimi
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@huawei.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@huawei.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.
>> + 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;
On 11/17/2022 6:18 PM, Casey Schaufler wrote:
> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.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.
>
> Smack uses multiple xattrs. All file system objects have a SMACK64
> attribute, which is used for access control. A program file may have
> a SMACK64EXEC attribute, which is the label the program will run with.
> A library may have a SMACK64MMAP attribute to restrict loading. A
> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> new object creation behavior.
>
> The point being that it may be more than a "nice idea" to support
> multiple xattrs. It's not a hypothetical situation.
Ok, that means that I have to change the number of xattrs reserved by
Smack in patch 3.
Thanks
Roberto
On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> >> +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.
Including this sort of info in the function description or as a comment
in the code would definitely simplify review.
security_check_compact_xattrs() is called in the loop after getting
each LSM's xattr(s). Only the current LSMs xattrs need to be
compressed, yet the loop goes to the maximum number of xattrs each
time. Just wondering if there is a way of improving it.
On Fri, 2022-11-18 at 10:32 +0100, Roberto Sassu wrote:
> > Smack uses multiple xattrs. All file system objects have a SMACK64
> > attribute, which is used for access control. A program file may have
> > a SMACK64EXEC attribute, which is the label the program will run with.
> > A library may have a SMACK64MMAP attribute to restrict loading. A
> > directory may have a SMACK64TRANSMUTE attribute, which modifies the
> > new object creation behavior.
> >
> > The point being that it may be more than a "nice idea" to support
> > multiple xattrs. It's not a hypothetical situation.
>
> Ok, that means that I have to change the number of xattrs reserved by
> Smack in patch 3.
Based on evm_config_default_xattrnames[], there are 4. There's the
original SMACK and these 3 additional ones.
Mimi
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@huawei.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@huawei.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;
>
On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
>>>> +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.
> Including this sort of info in the function description or as a comment
> in the code would definitely simplify review.
>
> security_check_compact_xattrs() is called in the loop after getting
> each LSM's xattr(s). Only the current LSMs xattrs need to be
> compressed, yet the loop goes to the maximum number of xattrs each
> time. Just wondering if there is a way of improving it.
At security module registration each module could identify how
many xattrs it uses. That number could be used to limit the range
of the loop. I have to do similar things for the forthcoming LSM
syscalls and module stacking beyond that.
>
On Fri, 2022-11-18 at 09:31 -0800, Casey Schaufler wrote:
> On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> > On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> > > > > +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.
> > Including this sort of info in the function description or as a comment
> > in the code would definitely simplify review.
> >
> > security_check_compact_xattrs() is called in the loop after getting
> > each LSM's xattr(s). Only the current LSMs xattrs need to be
> > compressed, yet the loop goes to the maximum number of xattrs each
> > time. Just wondering if there is a way of improving it.
>
> At security module registration each module could identify how
> many xattrs it uses. That number could be used to limit the range
> of the loop. I have to do similar things for the forthcoming LSM
> syscalls and module stacking beyond that.
Yes, blob_sizes.lbs_xattr contains the total number of xattrs requested
by LSMs. To stop the loop earlier, at the offset of the next LSM, we
would need to search the LSM's lsm_info, using the LSM name in
the security_hook_list structure. Although it is not optimal, not doing
it makes the code simpler. I could do that, if preferred.
Roberto
On Mon, 2022-11-21 at 14:29 +0100, Roberto Sassu wrote:
> On Fri, 2022-11-18 at 09:31 -0800, Casey Schaufler wrote:
> > On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> > > On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> > > > > > +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.
> > > Including this sort of info in the function description or as a comment
> > > in the code would definitely simplify review.
> > >
> > > security_check_compact_xattrs() is called in the loop after getting
> > > each LSM's xattr(s). Only the current LSMs xattrs need to be
> > > compressed, yet the loop goes to the maximum number of xattrs each
> > > time. Just wondering if there is a way of improving it.
> >
> > At security module registration each module could identify how
> > many xattrs it uses. That number could be used to limit the range
> > of the loop. I have to do similar things for the forthcoming LSM
> > syscalls and module stacking beyond that.
>
> Yes, blob_sizes.lbs_xattr contains the total number of xattrs requested
> by LSMs. To stop the loop earlier, at the offset of the next LSM, we
> would need to search the LSM's lsm_info, using the LSM name in
> the security_hook_list structure. Although it is not optimal, not doing
> it makes the code simpler. I could do that, if preferred.
Either way is fine, as long as the code is readable. At minimum add a
comment.
@@ -112,8 +112,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
- struct inode *dir, const struct qstr *qstr, const char **name,
- void **value, size_t *len)
+ struct inode *dir, const struct qstr *qstr, struct xattr *xattrs)
LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
const struct qstr *name, const struct inode *context_inode)
LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
@@ -229,18 +229,22 @@
* This hook is called by the fs code as part of the inode creation
* transaction and provides for atomic labeling of the inode, unlike
* the post_create/mkdir/... hooks called by the VFS. The hook function
- * is expected to allocate the name and value via kmalloc, with the caller
- * being responsible for calling kfree after using them.
+ * is expected to allocate the value via kmalloc, with the caller
+ * being responsible for calling kfree after using it.
* If the security module does not use security attributes or does
* not wish to put a security attribute on this particular inode,
* then it should return -EOPNOTSUPP to skip this processing.
* @inode contains the inode structure of the newly created inode.
* @dir contains the inode structure of the parent directory.
* @qstr contains the last path component of the new object
- * @name will be set to the allocated name suffix (e.g. selinux).
- * @value will be set to the allocated attribute value.
- * @len will be set to the length of the value.
- * Returns 0 if @name and @value have been successfully set,
+ * @xattrs contains the full array of xattrs provided by LSMs where
+ * ->name will be set to the name suffix (e.g. selinux).
+ * ->value will be set to the allocated attribute value.
+ * ->value_len will be set to the length of the value.
+ * Slots in @xattrs need to be reserved by LSMs by providing the number of
+ * the desired xattrs in the lbs_xattr field of the lsm_blob_sizes
+ * structure.
+ * Returns 0 if the requested slots in @xattrs have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
* @inode_init_security_anon:
@@ -1624,6 +1628,7 @@ struct lsm_blob_sizes {
int lbs_ipc;
int lbs_msg_msg;
int lbs_task;
+ int lbs_xattr;
};
/*
@@ -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)
+{
+ int i;
+
+ for (i = *checked_xattrs; i < num_xattrs; i++) {
+ 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);
+ 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;
@@ -104,6 +104,8 @@
#include "audit.h"
#include "avc_ss.h"
+#define SELINUX_INODE_INIT_XATTRS 1
+
struct selinux_state selinux_state;
/* SECMARK reference count */
@@ -2868,11 +2870,11 @@ static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
- const char **name,
- void **value, size_t *len)
+ struct xattr *xattrs)
{
const struct task_security_struct *tsec = selinux_cred(current_cred());
struct superblock_security_struct *sbsec;
+ struct xattr *xattr = NULL;
u32 newsid, clen;
int rc;
char *context;
@@ -2899,16 +2901,18 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
!(sbsec->flags & SBLABEL_MNT))
return -EOPNOTSUPP;
- if (name)
- *name = XATTR_SELINUX_SUFFIX;
+ if (xattrs)
+ xattr = xattrs + selinux_blob_sizes.lbs_xattr;
+
+ if (xattr) {
+ xattr->name = XATTR_SELINUX_SUFFIX;
- if (value && len) {
rc = security_sid_to_context_force(&selinux_state, newsid,
&context, &clen);
if (rc)
return rc;
- *value = context;
- *len = clen;
+ xattr->value = context;
+ xattr->value_len = clen;
}
return 0;
@@ -6900,6 +6904,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_ipc = sizeof(struct ipc_security_struct),
.lbs_msg_msg = sizeof(struct msg_security_struct),
.lbs_superblock = sizeof(struct superblock_security_struct),
+ .lbs_xattr = SELINUX_INODE_INIT_XATTRS,
};
#ifdef CONFIG_PERF_EVENTS
@@ -52,6 +52,8 @@
#define SMK_RECEIVING 1
#define SMK_SENDING 2
+#define SMACK_INODE_INIT_XATTRS 1
+
#ifdef SMACK_IPV6_PORT_LABELING
static DEFINE_MUTEX(smack_ipv6_lock);
static LIST_HEAD(smk_ipv6_port_list);
@@ -939,26 +941,27 @@ static int smack_inode_alloc_security(struct inode *inode)
* @inode: the newly created inode
* @dir: containing directory object
* @qstr: unused
- * @name: where to put the attribute name
- * @value: where to put the attribute value
- * @len: where to put the length of the attribute
+ * @xattrs: where to put the attribute
*
* Returns 0 if it all works out, -ENOMEM if there's no memory
*/
static int smack_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, const char **name,
- void **value, size_t *len)
+ const struct qstr *qstr,
+ struct xattr *xattrs)
{
struct inode_smack *issp = smack_inode(inode);
struct smack_known *skp = smk_of_current();
struct smack_known *isp = smk_of_inode(inode);
struct smack_known *dsp = smk_of_inode(dir);
+ struct xattr *xattr = NULL;
int may;
- if (name)
- *name = XATTR_SMACK_SUFFIX;
+ if (xattrs)
+ xattr = xattrs + smack_blob_sizes.lbs_xattr;
+
+ if (xattr) {
+ xattr->name = XATTR_SMACK_SUFFIX;
- if (value && len) {
rcu_read_lock();
may = smk_access_entry(skp->smk_known, dsp->smk_known,
&skp->smk_rules);
@@ -976,11 +979,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
issp->smk_flags |= SMK_INODE_CHANGED;
}
- *value = kstrdup(isp->smk_known, GFP_NOFS);
- if (*value == NULL)
+ xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
+ if (xattr->value == NULL)
return -ENOMEM;
- *len = strlen(isp->smk_known);
+ xattr->value_len = strlen(isp->smk_known);
}
return 0;
@@ -4785,6 +4788,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_ipc = sizeof(struct smack_known *),
.lbs_msg_msg = sizeof(struct smack_known *),
.lbs_superblock = sizeof(struct superblock_smack),
+ .lbs_xattr = SMACK_INODE_INIT_XATTRS,
};
static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {