EVM has recently been completely disabled on unsupported (e.g.,
overlayfs). This series now enables copy-up of "portable and immutable"
signatures on those filesystems and enables the enforcement of
"portable and immutable" as well as the "original" signatures on
previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
HMAC verification and generation remains disabled on those filesystems.
Regards,
Stefan
Stefan Berger (5):
security: allow finer granularity in permitting copy-up of security
xattrs
evm: Implement per signature type decision in
security_inode_copy_up_xattr
ima: Reset EVM status upon detecting changes to overlay backing file
evm: Use the real inode's metadata to calculate metadata hash
evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509
fs/overlayfs/copy_up.c | 2 +-
include/linux/evm.h | 10 +++++-
include/linux/lsm_hook_defs.h | 3 +-
include/linux/security.h | 4 +--
security/integrity/evm/evm_crypto.c | 2 +-
security/integrity/evm/evm_main.c | 48 +++++++++++++++++++++++------
security/integrity/ima/ima_main.c | 2 ++
security/security.c | 7 +++--
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
10 files changed, 62 insertions(+), 20 deletions(-)
--
2.43.0
To support portable and immutable signatures on otherwise unsupported
filesystems, determine the EVM signature type by the content of a file's
xattr. If the file has the appropriate signature then allow it to be
copied up. All other signature types are discarded as before.
Portable and immutable EVM signatures can be copied up by stacked file-
system since the metadata their signature covers does not include file-
system-specific data such as a file's inode number, generation, and UUID.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/evm/evm_main.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2555aa4501ae..22a5e26860ea 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -898,9 +898,30 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
{
- if (strcmp(name, XATTR_NAME_EVM) == 0)
- return 1; /* Discard */
- return -EOPNOTSUPP;
+ struct evm_ima_xattr_data *xattr_data = NULL;
+ int rc;
+
+ if (strcmp(name, XATTR_NAME_EVM) != 0)
+ return -EOPNOTSUPP;
+
+ /* first need to know the sig type */
+ rc = vfs_getxattr_alloc(&nop_mnt_idmap, src, XATTR_NAME_EVM,
+ (char **)&xattr_data, 0, GFP_NOFS);
+ if (rc <= 0)
+ return -EPERM;
+
+ switch (xattr_data->type) {
+ case EVM_XATTR_PORTABLE_DIGSIG:
+ rc = 0; /* allow copy-up */
+ break;
+ case EVM_XATTR_HMAC:
+ case EVM_IMA_XATTR_DIGSIG:
+ default:
+ rc = 1; /* discard */
+ }
+
+ kfree(xattr_data);
+ return rc;
}
/*
--
2.43.0
Copying up xattrs is solely based on the security xattr name. For finer
granularity add a dentry parameter to the security_inode_copy_up_xattr
hook definition, allowing decisions to be based on the xattr content as
well.
Signed-off-by: Stefan Berger <[email protected]>
---
fs/overlayfs/copy_up.c | 2 +-
include/linux/evm.h | 2 +-
include/linux/lsm_hook_defs.h | 3 ++-
include/linux/security.h | 4 ++--
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 7 ++++---
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
8 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b8e25ca51016..bd9ddcefb7a7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
if (ovl_is_private_xattr(sb, name))
continue;
- error = security_inode_copy_up_xattr(name);
+ error = security_inode_copy_up_xattr(old, name);
if (error < 0 && error != -EOPNOTSUPP)
break;
if (error == 1) {
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 36ec884320d9..d8c0343436b8 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -31,7 +31,7 @@ extern void evm_inode_post_setxattr(struct dentry *dentry,
const char *xattr_name,
const void *xattr_value,
size_t xattr_value_len);
-extern int evm_inode_copy_up_xattr(const char *name);
+extern int evm_inode_copy_up_xattr(struct dentry *dentry, const char *name);
extern int evm_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *xattr_name);
extern void evm_inode_post_removexattr(struct dentry *dentry,
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 185924c56378..7dd61f51d84a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -163,7 +163,8 @@ LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
size_t buffer_size)
LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
-LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
+LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
+ const char *name)
LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
struct kernfs_node *kn)
LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..9fc9ca6284d6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -387,7 +387,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
void security_inode_getsecid(struct inode *inode, u32 *secid);
int security_inode_copy_up(struct dentry *src, struct cred **new);
-int security_inode_copy_up_xattr(const char *name);
+int security_inode_copy_up_xattr(struct dentry *src, const char *name);
int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
@@ -980,7 +980,7 @@ static inline int security_kernfs_init_security(struct kernfs_node *kn_dir,
return 0;
}
-static inline int security_inode_copy_up_xattr(const char *name)
+static inline int security_inode_copy_up_xattr(struct dentry *src, const char *name)
{
return -EOPNOTSUPP;
}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cc7956d7878b..2555aa4501ae 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -896,7 +896,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
evm_update_evmxattr(dentry, NULL, NULL, 0);
}
-int evm_inode_copy_up_xattr(const char *name)
+int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
{
if (strcmp(name, XATTR_NAME_EVM) == 0)
return 1; /* Discard */
diff --git a/security/security.c b/security/security.c
index 0144a98d3712..ee63863c1dc0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2596,6 +2596,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
/**
* security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
+ * @src: union dentry of copy-up file
* @name: xattr name
*
* Filter the xattrs being copied up when a unioned file is copied up from a
@@ -2606,7 +2607,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
* if the security module does not know about attribute, or a negative
* error code to abort the copy up.
*/
-int security_inode_copy_up_xattr(const char *name)
+int security_inode_copy_up_xattr(struct dentry *src, const char *name)
{
struct security_hook_list *hp;
int rc;
@@ -2618,12 +2619,12 @@ int security_inode_copy_up_xattr(const char *name)
*/
hlist_for_each_entry(hp,
&security_hook_heads.inode_copy_up_xattr, list) {
- rc = hp->hook.inode_copy_up_xattr(name);
+ rc = hp->hook.inode_copy_up_xattr(src, name);
if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
return rc;
}
- return evm_inode_copy_up_xattr(name);
+ return evm_inode_copy_up_xattr(src, name);
}
EXPORT_SYMBOL(security_inode_copy_up_xattr);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..ebb8876837c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3530,7 +3530,7 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
return 0;
}
-static int selinux_inode_copy_up_xattr(const char *name)
+static int selinux_inode_copy_up_xattr(struct dentry *dentry, const char *name)
{
/* The copy_up hook above sets the initial context on an inode, but we
* don't then want to overwrite it by blindly copying all the lower
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..bffca165f07f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4873,7 +4873,7 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
return 0;
}
-static int smack_inode_copy_up_xattr(const char *name)
+static int smack_inode_copy_up_xattr(struct dentry *src, const char *name)
{
/*
* Return 1 if this is the smack access Smack attribute.
--
2.43.0
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>
> EVM has recently been completely disabled on unsupported (e.g.,
> overlayfs). This series now enables copy-up of "portable and immutable"
> signatures on those filesystems and enables the enforcement of
> "portable and immutable" as well as the "original" signatures on
> previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
> HMAC verification and generation remains disabled on those filesystems.
>
I am missing a high level description of what is in those "portable
and immutable"
signatures and how those signatures remain valid across copy up.
Thanks,
Amir.
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>
> Copying up xattrs is solely based on the security xattr name. For finer
> granularity add a dentry parameter to the security_inode_copy_up_xattr
> hook definition, allowing decisions to be based on the xattr content as
> well.
>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> fs/overlayfs/copy_up.c | 2 +-
> include/linux/evm.h | 2 +-
> include/linux/lsm_hook_defs.h | 3 ++-
> include/linux/security.h | 4 ++--
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 7 ++++---
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 8 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b8e25ca51016..bd9ddcefb7a7 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> if (ovl_is_private_xattr(sb, name))
> continue;
>
> - error = security_inode_copy_up_xattr(name);
> + error = security_inode_copy_up_xattr(old, name);
What do you think about:
error = security_inode_copy_up_xattr(name, NULL, 0);
and then later...
error = security_inode_copy_up_xattr(name, value, size);
I am asking because overlayfs uses mnt_idmap(path->mnt) and you
have used nop_mnt_idmap inside evm hook.
this does not look right to me?
Thanks,
Amir.
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>
> To support portable and immutable signatures on otherwise unsupported
> filesystems, determine the EVM signature type by the content of a file's
> xattr. If the file has the appropriate signature then allow it to be
> copied up. All other signature types are discarded as before.
>
> Portable and immutable EVM signatures can be copied up by stacked file-
> system since the metadata their signature covers does not include file-
> system-specific data such as a file's inode number, generation, and UUID.
>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> security/integrity/evm/evm_main.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2555aa4501ae..22a5e26860ea 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -898,9 +898,30 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>
> int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
> {
> - if (strcmp(name, XATTR_NAME_EVM) == 0)
> - return 1; /* Discard */
> - return -EOPNOTSUPP;
> + struct evm_ima_xattr_data *xattr_data = NULL;
> + int rc;
> +
> + if (strcmp(name, XATTR_NAME_EVM) != 0)
> + return -EOPNOTSUPP;
> +
> + /* first need to know the sig type */
> + rc = vfs_getxattr_alloc(&nop_mnt_idmap, src, XATTR_NAME_EVM,
> + (char **)&xattr_data, 0, GFP_NOFS);
See my suggestion for post-getxattr hook:
security_inode_copy_up_xattr(name, value, size)
to avoid using nop_mnt_idmap here.
Unless it is fine to use nop_mnt_idmap in this context? not sure.
Thanks,
Amir.
On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
> >
> > Copying up xattrs is solely based on the security xattr name. For finer
> > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > hook definition, allowing decisions to be based on the xattr content as
> > well.
> >
> > Signed-off-by: Stefan Berger <[email protected]>
> > ---
> > fs/overlayfs/copy_up.c | 2 +-
> > include/linux/evm.h | 2 +-
> > include/linux/lsm_hook_defs.h | 3 ++-
> > include/linux/security.h | 4 ++--
> > security/integrity/evm/evm_main.c | 2 +-
> > security/security.c | 7 ++++---
> > security/selinux/hooks.c | 2 +-
> > security/smack/smack_lsm.c | 2 +-
> > 8 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index b8e25ca51016..bd9ddcefb7a7 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > if (ovl_is_private_xattr(sb, name))
> > continue;
> >
> > - error = security_inode_copy_up_xattr(name);
> > + error = security_inode_copy_up_xattr(old, name);
>
> What do you think about:
>
> error = security_inode_copy_up_xattr(name, NULL, 0);
>
> and then later...
>
> error = security_inode_copy_up_xattr(name, value, size);
>
> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> have used nop_mnt_idmap inside evm hook.
> this does not look right to me?
So it's relevant if they interact with xattrs that care about the
idmapping and that's POSIX ACLs and fscaps. And only if they perform
permission checks such as posix_acl_update_mode() or something. IOW, it
depends on what exactly EVM is doing.
IIRC, I already added custom security_*() hooks specifically for POSIX
ACLs as they can't be retrieved through vfs_xattr*() helpers anymore.
On 1/31/24 09:25, Christian Brauner wrote:
> On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
>> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>>>
>>> Copying up xattrs is solely based on the security xattr name. For finer
>>> granularity add a dentry parameter to the security_inode_copy_up_xattr
>>> hook definition, allowing decisions to be based on the xattr content as
>>> well.
>>>
>>> Signed-off-by: Stefan Berger <[email protected]>
>>> ---
>>> fs/overlayfs/copy_up.c | 2 +-
>>> include/linux/evm.h | 2 +-
>>> include/linux/lsm_hook_defs.h | 3 ++-
>>> include/linux/security.h | 4 ++--
>>> security/integrity/evm/evm_main.c | 2 +-
>>> security/security.c | 7 ++++---
>>> security/selinux/hooks.c | 2 +-
>>> security/smack/smack_lsm.c | 2 +-
>>> 8 files changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index b8e25ca51016..bd9ddcefb7a7 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>>> if (ovl_is_private_xattr(sb, name))
>>> continue;
>>>
>>> - error = security_inode_copy_up_xattr(name);
>>> + error = security_inode_copy_up_xattr(old, name);
>>
>> What do you think about:
>>
>> error = security_inode_copy_up_xattr(name, NULL, 0);
>>
>> and then later...
>>
>> error = security_inode_copy_up_xattr(name, value, size);
>>
>> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
>> have used nop_mnt_idmap inside evm hook.
>> this does not look right to me?
>
> So it's relevant if they interact with xattrs that care about the
> idmapping and that's POSIX ACLs and fscaps. And only if they perform
> permission checks such as posix_acl_update_mode() or something. IOW, it
> depends on what exactly EVM is doing.
In 2/5 we are reading the value of security.evm to look at its contents.
>
> IIRC, I already added custom security_*() hooks specifically for POSIX
> ACLs as they can't be retrieved through vfs_xattr*() helpers anymore.
On 1/31/24 08:18, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>>
>> EVM has recently been completely disabled on unsupported (e.g.,
>> overlayfs). This series now enables copy-up of "portable and immutable"
>> signatures on those filesystems and enables the enforcement of
>> "portable and immutable" as well as the "original" signatures on
>> previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
>> HMAC verification and generation remains disabled on those filesystems.
>>
>
> I am missing a high level description of what is in those "portable
> and immutable"
> signatures and how those signatures remain valid across copy up.
>
From 2/5:
"Portable and immutable EVM signatures can be copied up by stacked file-
system since the metadata their signature covers does not include file-
system-specific data such as a file's inode number, generation, and UUID."
Instead, the signatures cover file metadata such as file mode bits, uid,
and gid as well as xattrs, which can all be preserved unchanged across a
copy-up.
Reference:
https://elixir.bootlin.com/linux/v6.7.2/source/security/integrity/evm/evm_crypto.c#L169
> Thanks,
> Amir.
>
Hi Stefan,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc2 next-20240131]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/security-allow-finer-granularity-in-permitting-copy-up-of-security-xattrs/20240131-054854
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20240130214620.3155380-2-stefanb%40linux.ibm.com
patch subject: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs
config: i386-buildonly-randconfig-002-20240131 (https://download.01.org/0day-ci/archive/20240201/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240201/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> security/security.c:2627:38: error: too many arguments to function call, expected single argument 'name', have 2 arguments
2627 | return evm_inode_copy_up_xattr(src, name);
| ~~~~~~~~~~~~~~~~~~~~~~~ ^~~~
include/linux/evm.h:121:20: note: 'evm_inode_copy_up_xattr' declared here
121 | static inline int evm_inode_copy_up_xattr(const char *name)
| ^ ~~~~~~~~~~~~~~~~
1 error generated.
vim +/name +2627 security/security.c
2596
2597 /**
2598 * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
2599 * @src: union dentry of copy-up file
2600 * @name: xattr name
2601 *
2602 * Filter the xattrs being copied up when a unioned file is copied up from a
2603 * lower layer to the union/overlay layer. The caller is responsible for
2604 * reading and writing the xattrs, this hook is merely a filter.
2605 *
2606 * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
2607 * if the security module does not know about attribute, or a negative
2608 * error code to abort the copy up.
2609 */
2610 int security_inode_copy_up_xattr(struct dentry *src, const char *name)
2611 {
2612 struct security_hook_list *hp;
2613 int rc;
2614
2615 /*
2616 * The implementation can return 0 (accept the xattr), 1 (discard the
2617 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
2618 * any other error code in case of an error.
2619 */
2620 hlist_for_each_entry(hp,
2621 &security_hook_heads.inode_copy_up_xattr, list) {
2622 rc = hp->hook.inode_copy_up_xattr(src, name);
2623 if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
2624 return rc;
2625 }
2626
> 2627 return evm_inode_copy_up_xattr(src, name);
2628 }
2629 EXPORT_SYMBOL(security_inode_copy_up_xattr);
2630
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Stefan,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc2 next-20240131]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/security-allow-finer-granularity-in-permitting-copy-up-of-security-xattrs/20240131-054854
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20240130214620.3155380-2-stefanb%40linux.ibm.com
patch subject: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240201/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240201/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
security/security.c: In function 'security_inode_copy_up_xattr':
>> security/security.c:2627:40: error: passing argument 1 of 'evm_inode_copy_up_xattr' from incompatible pointer type [-Werror=incompatible-pointer-types]
2627 | return evm_inode_copy_up_xattr(src, name);
| ^~~
| |
| struct dentry *
In file included from security/security.c:24:
include/linux/evm.h:121:56: note: expected 'const char *' but argument is of type 'struct dentry *'
121 | static inline int evm_inode_copy_up_xattr(const char *name)
| ~~~~~~~~~~~~^~~~
>> security/security.c:2627:16: error: too many arguments to function 'evm_inode_copy_up_xattr'
2627 | return evm_inode_copy_up_xattr(src, name);
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from security/security.c:24:
include/linux/evm.h:121:20: note: declared here
121 | static inline int evm_inode_copy_up_xattr(const char *name)
| ^~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/evm_inode_copy_up_xattr +2627 security/security.c
2596
2597 /**
2598 * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
2599 * @src: union dentry of copy-up file
2600 * @name: xattr name
2601 *
2602 * Filter the xattrs being copied up when a unioned file is copied up from a
2603 * lower layer to the union/overlay layer. The caller is responsible for
2604 * reading and writing the xattrs, this hook is merely a filter.
2605 *
2606 * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
2607 * if the security module does not know about attribute, or a negative
2608 * error code to abort the copy up.
2609 */
2610 int security_inode_copy_up_xattr(struct dentry *src, const char *name)
2611 {
2612 struct security_hook_list *hp;
2613 int rc;
2614
2615 /*
2616 * The implementation can return 0 (accept the xattr), 1 (discard the
2617 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
2618 * any other error code in case of an error.
2619 */
2620 hlist_for_each_entry(hp,
2621 &security_hook_heads.inode_copy_up_xattr, list) {
2622 rc = hp->hook.inode_copy_up_xattr(src, name);
2623 if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
2624 return rc;
2625 }
2626
> 2627 return evm_inode_copy_up_xattr(src, name);
2628 }
2629 EXPORT_SYMBOL(security_inode_copy_up_xattr);
2630
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
>
>
> On 1/31/24 09:25, Christian Brauner wrote:
> > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
> > > >
> > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > hook definition, allowing decisions to be based on the xattr content as
> > > > well.
> > > >
> > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > ---
> > > > fs/overlayfs/copy_up.c | 2 +-
> > > > include/linux/evm.h | 2 +-
> > > > include/linux/lsm_hook_defs.h | 3 ++-
> > > > include/linux/security.h | 4 ++--
> > > > security/integrity/evm/evm_main.c | 2 +-
> > > > security/security.c | 7 ++++---
> > > > security/selinux/hooks.c | 2 +-
> > > > security/smack/smack_lsm.c | 2 +-
> > > > 8 files changed, 13 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > --- a/fs/overlayfs/copy_up.c
> > > > +++ b/fs/overlayfs/copy_up.c
> > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > > if (ovl_is_private_xattr(sb, name))
> > > > continue;
> > > >
> > > > - error = security_inode_copy_up_xattr(name);
> > > > + error = security_inode_copy_up_xattr(old, name);
> > >
> > > What do you think about:
> > >
> > > error = security_inode_copy_up_xattr(name, NULL, 0);
> > >
> > > and then later...
> > >
> > > error = security_inode_copy_up_xattr(name, value, size);
> > >
> > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > have used nop_mnt_idmap inside evm hook.
> > > this does not look right to me?
> >
> > So it's relevant if they interact with xattrs that care about the
> > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > permission checks such as posix_acl_update_mode() or something. IOW, it
> > depends on what exactly EVM is doing.
>
> In 2/5 we are reading the value of security.evm to look at its contents.
I'm not sure what this is supposed to be telling me in relation to the
original question though. :) security.evm doesn't store any {g,u}id
information afaict. IOW, it shouldn't matter?
On Thu, Feb 1, 2024 at 3:35 PM Christian Brauner <[email protected]> wrote:
>
> On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
> >
> >
> > On 1/31/24 09:25, Christian Brauner wrote:
> > > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
> > > > >
> > > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > > hook definition, allowing decisions to be based on the xattr content as
> > > > > well.
> > > > >
> > > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > > ---
> > > > > fs/overlayfs/copy_up.c | 2 +-
> > > > > include/linux/evm.h | 2 +-
> > > > > include/linux/lsm_hook_defs.h | 3 ++-
> > > > > include/linux/security.h | 4 ++--
> > > > > security/integrity/evm/evm_main.c | 2 +-
> > > > > security/security.c | 7 ++++---
> > > > > security/selinux/hooks.c | 2 +-
> > > > > security/smack/smack_lsm.c | 2 +-
> > > > > 8 files changed, 13 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > > --- a/fs/overlayfs/copy_up.c
> > > > > +++ b/fs/overlayfs/copy_up.c
> > > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > > > if (ovl_is_private_xattr(sb, name))
> > > > > continue;
> > > > >
> > > > > - error = security_inode_copy_up_xattr(name);
> > > > > + error = security_inode_copy_up_xattr(old, name);
> > > >
> > > > What do you think about:
> > > >
> > > > error = security_inode_copy_up_xattr(name, NULL, 0);
> > > >
> > > > and then later...
> > > >
> > > > error = security_inode_copy_up_xattr(name, value, size);
> > > >
> > > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > > have used nop_mnt_idmap inside evm hook.
> > > > this does not look right to me?
> > >
> > > So it's relevant if they interact with xattrs that care about the
> > > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > > permission checks such as posix_acl_update_mode() or something. IOW, it
> > > depends on what exactly EVM is doing.
> >
> > In 2/5 we are reading the value of security.evm to look at its contents.
>
> I'm not sure what this is supposed to be telling me in relation to the
> original question though. :) security.evm doesn't store any {g,u}id
> information afaict. IOW, it shouldn't matter?
But it does. in evm_calc_hmac_or_hash() => hmac_add_misc():
hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
I guess as far as EVM is concerned, it should always be interested in the
absolute uig/gid values of the inode.
Thanks,
Amir.
On 1/31/24 08:25, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
>>
>> Copying up xattrs is solely based on the security xattr name. For finer
>> granularity add a dentry parameter to the security_inode_copy_up_xattr
>> hook definition, allowing decisions to be based on the xattr content as
>> well.
>>
>> Signed-off-by: Stefan Berger <[email protected]>
>> ---
>> fs/overlayfs/copy_up.c | 2 +-
>> include/linux/evm.h | 2 +-
>> include/linux/lsm_hook_defs.h | 3 ++-
>> include/linux/security.h | 4 ++--
>> security/integrity/evm/evm_main.c | 2 +-
>> security/security.c | 7 ++++---
>> security/selinux/hooks.c | 2 +-
>> security/smack/smack_lsm.c | 2 +-
>> 8 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index b8e25ca51016..bd9ddcefb7a7 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>> if (ovl_is_private_xattr(sb, name))
>> continue;
>>
>> - error = security_inode_copy_up_xattr(name);
>> + error = security_inode_copy_up_xattr(old, name);
>
> What do you think about:
>
> error = security_inode_copy_up_xattr(name, NULL, 0);
We need 'old'.
>
> and then later...
>
> error = security_inode_copy_up_xattr(name, value, size);
Are these parameter used to first query for the necessary size of the
buffer and then provide the buffer to fill it? Or should the function
rather take an existing buffer and realloc it if necessary and place the
value of the xattr into it? Unfortunately this function currently
returns '1' for 'discard', so returning the size of the xattr value from
it maybe not ideal but it would require maybe yet another parameter that
indicates what the size of the xattr value is.
Stefan
>
> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> have used nop_mnt_idmap inside evm hook.
> this does not look right to me?
>
> Thanks,
> Amir.
On Thu, Feb 01, 2024 at 04:18:32PM +0200, Amir Goldstein wrote:
> On Thu, Feb 1, 2024 at 3:35 PM Christian Brauner <[email protected]> wrote:
> >
> > On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
> > >
> > >
> > > On 1/31/24 09:25, Christian Brauner wrote:
> > > > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> wrote:
> > > > > >
> > > > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > > > hook definition, allowing decisions to be based on the xattr content as
> > > > > > well.
> > > > > >
> > > > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > > > ---
> > > > > > fs/overlayfs/copy_up.c | 2 +-
> > > > > > include/linux/evm.h | 2 +-
> > > > > > include/linux/lsm_hook_defs.h | 3 ++-
> > > > > > include/linux/security.h | 4 ++--
> > > > > > security/integrity/evm/evm_main.c | 2 +-
> > > > > > security/security.c | 7 ++++---
> > > > > > security/selinux/hooks.c | 2 +-
> > > > > > security/smack/smack_lsm.c | 2 +-
> > > > > > 8 files changed, 13 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > > > --- a/fs/overlayfs/copy_up.c
> > > > > > +++ b/fs/overlayfs/copy_up.c
> > > > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > > > > if (ovl_is_private_xattr(sb, name))
> > > > > > continue;
> > > > > >
> > > > > > - error = security_inode_copy_up_xattr(name);
> > > > > > + error = security_inode_copy_up_xattr(old, name);
> > > > >
> > > > > What do you think about:
> > > > >
> > > > > error = security_inode_copy_up_xattr(name, NULL, 0);
> > > > >
> > > > > and then later...
> > > > >
> > > > > error = security_inode_copy_up_xattr(name, value, size);
> > > > >
> > > > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > > > have used nop_mnt_idmap inside evm hook.
> > > > > this does not look right to me?
> > > >
> > > > So it's relevant if they interact with xattrs that care about the
> > > > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > > > permission checks such as posix_acl_update_mode() or something. IOW, it
> > > > depends on what exactly EVM is doing.
> > >
> > > In 2/5 we are reading the value of security.evm to look at its contents.
> >
> > I'm not sure what this is supposed to be telling me in relation to the
> > original question though. :) security.evm doesn't store any {g,u}id
> > information afaict. IOW, it shouldn't matter?
>
> But it does. in evm_calc_hmac_or_hash() => hmac_add_misc():
>
> hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
> hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
>
> I guess as far as EVM is concerned, it should always be interested in the
> absolute uig/gid values of the inode.
There we go, thanks Amir. Yes, these EVM values can't be relative to any
idmappings. If inode->i_uid has kuid 100000 and 100000 maps to zero in
the caller's user namespace then you'd be storing hmac_misc.{g,u}id
zero. That would be problematic as it would give the impression that
real root caused that hmac to be written. So this really needs to store
100000 to make it clear that this was an unprivileged user that created
these values.