linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/30] acl: add vfs posix acl api
@ 2022-09-26 14:07 Christian Brauner
  2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Christian Brauner @ 2022-09-26 14:07 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Christian Brauner (Microsoft),
	Seth Forshee, Christoph Hellwig, Linus Torvalds, Al Viro,
	v9fs-developer, linux-cifs, linux-integrity,
	linux-security-module

From: "Christian Brauner (Microsoft)" <brauner@kernel.org>

Hey everyone,

/* v2 */
This fixes various things pointed out during review. The individual
commits contain more details were appropriate.

As we discussed and seen multiple times the current state of how posix
acls are handled isn't nice and comes with a lot of problems. For a long
and detailed explanation for just some of the issues [1] provides a good
summary.

The current way of handling posix acls via the generic xattr api is
error prone, hard to maintain, and type unsafe for the vfs until we call
into the filesystem's dedicated get and set inode operations.

It is already the case that posix acls are special-cased to death all
the way through the vfs. There are an uncounted number of hacks that
operate on the uapi posix acl struct instead of the dedicated vfs struct
posix_acl. And the vfs must be involved in order to interpret and fixup
posix acls before storing them to the backing store, caching them,
reporting them to userspace, or for permission checking.

Currently a range of hacks and duct tape exist to make this work. As
with most things this is really no ones fault it's just something that
happened over time. But the code is hard to understand and difficult
to maintain and one is constantly at risk of introducing bugs and
regressions when having to touch it.

Instead of continuing to hack posix acls through the xattr handlers this
series builds a dedicated posix acl api solely around the get and set
inode operations. Going forward, the vfs_get_acl(), vfs_remove_acl(),
and vfs_set_acl() helpers must be used in order to interact with posix
acls. They operate directly on the vfs internal struct posix_acl instead
of abusing the uapi posix acl struct as we currently do. In the end this
removes all of the hackiness, makes the codepaths easier to maintain,
and gets us type safety.

This series passes the LTP and xfstests suites without any regressions.
For xfstests the following combinations were tested:

* xfs
* ext4
* btrfs
* overlayfs
* overlayfs on top of idmapped mounts

For people wanting to run their own xfstests I'd recommend to shorten
their test runs via:

./check -g acl,attr,cap,idmapped,io_uring,perms,subvol,unlink

I would appreciate if the 9p and cifs folks could run any posix acl
related tests as I have no setup to really do this without causing me a
lot of pain.

Very likely there's a lot more simplifications for posix acls that we
can make in the future if the basic api has made it.

A few implementation details:

* The series makes sure to retain exactly the same security and
  integrity module permission checks. See [2] for annotated callchains.
  Especially for the integrity modules this api is a win because right
  now they convert the uapi posix acl struct passed to them via a void
  pointer into the vfs struct posix_acl format to perform permission
  checking on the mode.

  There's a new dedicated security hook for setting posix acls which
  passes the vfs struct posix_acl not a void pointer. Basing checking on
  the posix acl stored in the uapi format is really unreliable. The vfs
  currently hacks around directly in the uapi struct storing values that
  frankly the security and integrity modules can't correctly interpret
  as evidenced by bugs we reported and fixed in this area. It's not
  necessarily even their fault it's just that the format we provide to
  them is sub optimal.

* Some filesystems like 9p and cifs need access to the dentry in order
  to get and set posix acls which is why they either only partially or
  not even at all implement get and set inode operations. For example,
  cifs allows setxattr() and getxattr() operations but doesn't allow
  permission checking based on posix acls because it can't implement a
  get acl inode operation.

  Thus, this patch series updates the set acl inode operation to take a
  dentry instead of an inode argument. However, for the get acl inode
  operation we can't do this as the old get acl method is called in
  e.g., generic_permission() and inode_permission(). These helpers in
  turn are called in various filesystem's permission inode operation. So
  passing a dentry argument to the old get acl inode operation would
  amount to passing a dentry to the permission inode operation which we
  shouldn't and probably can't do.

  So instead of extending the existing inode operation Christoph
  suggested to add a new one. He also requested to ensure that the get
  and set acl inode operation taking a dentry are consistently named. So
  for this version the old get acl operation is renamed to
  ->get_inode_acl() and a new ->get_acl() inode operation taking a
  dentry is added. With this we can give both 9p and cifs get and set
  acl inode operations and in turn remove their complex custom posix
  xattr handlers.

* I've done a full audit of every codepaths using variant of the
  current generic xattr api to get and set posix acls and surprisingly
  it isn't that many places. There's of course always a chance that I
  might have missed some and I'm sure we'll find them soon enough.

  The crucial codepaths to be converted are obviously stacking
  filesystems such as ecryptfs and overlayfs.

  For a list of all callers currently using generic xattr api helpers
  see [2] including comments whether they support posix acls or not.

* The old vfs generic posix acl infrastructure doesn't obey
  the create and replace semantics promised on the setxattr(2) manpage.
  This patch series doesn't address this. It really is something we
  should revisit later though.

The patch series is roughly organized as follows:

// intended to be a non-functional change
1. Change existing set acl inode operation to take a dentry argument.

// intended to be a non-functional change
2. Rename existing get acl method.

// intended to be a non-functional change
3. Implement get and set acl inode operations for filesystems that
   couldn't implement one before because of the missing dentry. That's
   mostly 9p and cifs.

// intended to be a non-functional change
4. Build posix acl api, i.e., add vfs_get_acl(), vfs_remove_acl(), and
   vfs_set_acl() including security and integrity hooks.

// intended to be a non-functional change
5. Implement get and set acl inode operations for stacking filesystems.

// semantical change
6. Switch posix acl handling in stacking filesystems to new posix acl
   api now that all filesystems it can stack upon support it.

// semantical change
7. Switch vfs to new posix acl api

8. Remove all now unused helpers

The series can be pulled from:

https://gitlab.com/brauner/linux/-/commits/fs.acl.rework
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework

The series contains a few preliminary patches which are scheduled for
the next merge window. It was just easier to base the series on top of
them. But if you pull this branch you'll get them included.

I've been working on this for a while and before going any further it'd
be nice to get some reviews. I think that it should be fine to have get
and set acl inode operations that operate on the dentry at least nothing
stuck out immediately that would prevent this. But obviously having
other people point out issues with that would be helpful.

Thanks to Seth for a lot of good discussion around this and
encouragement and input from Christoph.

[1]: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org
[2]: https://gist.github.com/brauner/12c795b93a05dc3b3056b1982549a633

v1: https://lore.kernel.org/linux-cifs/20220922151728.1557914-1-brauner@kernel.org

Thanks!
Christian

Christian Brauner (30):
  orangefs: rework posix acl handling when creating new filesystem
    objects
  fs: pass dentry to set acl method
  fs: rename current get acl method
  fs: add new get acl method
  cifs: implement get acl method
  cifs: implement set acl method
  9p: implement get acl method
  9p: implement set acl method
  acl: add vfs_set_acl()
  security: add set acl hook
  selinux: implement set acl hook
  smack: implement set acl hook
  evm: implement set acl hook
  acl: use set acl hook
  evm: add post set acl hook
  acl: add vfs_get_acl()
  acl: add vfs_remove_acl()
  evm: simplify evm_xattr_acl_change()
  ksmbd: use vfs_remove_acl()
  ecryptfs: implement get acl method
  ecryptfs: implement set acl method
  ovl: implement get acl method
  ovl: implement set acl method
  ovl: use posix acl api
  xattr: use posix acl api
  ecryptfs: use stub posix acl handlers
  ovl: use stub posix acl handlers
  cifs: use stub posix acl handlers
  9p: use stub posix acl handlers
  acl: remove a slew of now unused helpers

 Documentation/filesystems/locking.rst |   4 +-
 Documentation/filesystems/porting.rst |   4 +-
 Documentation/filesystems/vfs.rst     |   3 +-
 fs/9p/acl.c                           | 295 +++++------
 fs/9p/acl.h                           |   8 +-
 fs/9p/vfs_inode_dotl.c                |   4 +
 fs/9p/xattr.c                         |   7 +-
 fs/9p/xattr.h                         |   2 -
 fs/bad_inode.c                        |   4 +-
 fs/btrfs/acl.c                        |   3 +-
 fs/btrfs/ctree.h                      |   2 +-
 fs/btrfs/inode.c                      |   8 +-
 fs/ceph/acl.c                         |   3 +-
 fs/ceph/dir.c                         |   2 +-
 fs/ceph/inode.c                       |   4 +-
 fs/ceph/super.h                       |   2 +-
 fs/cifs/cifsacl.c                     | 141 ++++++
 fs/cifs/cifsfs.c                      |   4 +
 fs/cifs/cifsproto.h                   |  20 +-
 fs/cifs/cifssmb.c                     | 206 +++++---
 fs/cifs/xattr.c                       |  68 +--
 fs/ecryptfs/inode.c                   |  32 ++
 fs/erofs/inode.c                      |   6 +-
 fs/erofs/namei.c                      |   2 +-
 fs/ext2/acl.c                         |   3 +-
 fs/ext2/acl.h                         |   2 +-
 fs/ext2/file.c                        |   2 +-
 fs/ext2/inode.c                       |   2 +-
 fs/ext2/namei.c                       |   4 +-
 fs/ext4/acl.c                         |   3 +-
 fs/ext4/acl.h                         |   2 +-
 fs/ext4/file.c                        |   2 +-
 fs/ext4/inode.c                       |   2 +-
 fs/ext4/namei.c                       |   4 +-
 fs/f2fs/acl.c                         |   4 +-
 fs/f2fs/acl.h                         |   2 +-
 fs/f2fs/file.c                        |   4 +-
 fs/f2fs/namei.c                       |   4 +-
 fs/fuse/acl.c                         |   3 +-
 fs/fuse/dir.c                         |   4 +-
 fs/fuse/fuse_i.h                      |   2 +-
 fs/gfs2/acl.c                         |   3 +-
 fs/gfs2/acl.h                         |   2 +-
 fs/gfs2/inode.c                       |   6 +-
 fs/internal.h                         |   1 +
 fs/jffs2/acl.c                        |   3 +-
 fs/jffs2/acl.h                        |   2 +-
 fs/jffs2/dir.c                        |   2 +-
 fs/jffs2/file.c                       |   2 +-
 fs/jffs2/fs.c                         |   2 +-
 fs/jfs/acl.c                          |   3 +-
 fs/jfs/file.c                         |   4 +-
 fs/jfs/jfs_acl.h                      |   2 +-
 fs/jfs/namei.c                        |   2 +-
 fs/ksmbd/smb2pdu.c                    |   4 +-
 fs/ksmbd/smbacl.c                     |   4 +-
 fs/ksmbd/vfs.c                        |  17 +-
 fs/ksmbd/vfs.h                        |   4 +-
 fs/namei.c                            |   2 +-
 fs/nfs/nfs3_fs.h                      |   2 +-
 fs/nfs/nfs3acl.c                      |   3 +-
 fs/nfs/nfs3proc.c                     |   4 +-
 fs/nfsd/nfs2acl.c                     |   4 +-
 fs/nfsd/nfs3acl.c                     |   4 +-
 fs/nfsd/vfs.c                         |   4 +-
 fs/ntfs3/file.c                       |   4 +-
 fs/ntfs3/namei.c                      |   4 +-
 fs/ntfs3/ntfs_fs.h                    |   4 +-
 fs/ntfs3/xattr.c                      |   9 +-
 fs/ocfs2/acl.c                        |   3 +-
 fs/ocfs2/acl.h                        |   2 +-
 fs/ocfs2/file.c                       |   4 +-
 fs/ocfs2/namei.c                      |   2 +-
 fs/orangefs/acl.c                     |  47 +-
 fs/orangefs/inode.c                   |  47 +-
 fs/orangefs/namei.c                   |   2 +-
 fs/orangefs/orangefs-kernel.h         |   9 +-
 fs/orangefs/orangefs-utils.c          |  12 +-
 fs/overlayfs/copy_up.c                |  38 ++
 fs/overlayfs/dir.c                    |  22 +-
 fs/overlayfs/inode.c                  | 151 +++++-
 fs/overlayfs/overlayfs.h              |  34 +-
 fs/overlayfs/super.c                  | 107 +---
 fs/posix_acl.c                        | 681 +++++++++++++-------------
 fs/reiserfs/acl.h                     |   6 +-
 fs/reiserfs/file.c                    |   2 +-
 fs/reiserfs/inode.c                   |   2 +-
 fs/reiserfs/namei.c                   |   4 +-
 fs/reiserfs/xattr_acl.c               |   9 +-
 fs/xattr.c                            |  78 ++-
 fs/xfs/xfs_acl.c                      |   3 +-
 fs/xfs/xfs_acl.h                      |   2 +-
 fs/xfs/xfs_iops.c                     |  16 +-
 include/linux/evm.h                   |  23 +
 include/linux/fs.h                    |  10 +-
 include/linux/lsm_hook_defs.h         |   2 +
 include/linux/lsm_hooks.h             |   4 +
 include/linux/posix_acl.h             |  39 +-
 include/linux/posix_acl_xattr.h       |  43 +-
 include/linux/security.h              |  11 +
 include/linux/xattr.h                 |   8 +
 io_uring/xattr.c                      |   2 +
 mm/shmem.c                            |   2 +-
 security/integrity/evm/evm_main.c     | 128 +++--
 security/security.c                   |  16 +
 security/selinux/hooks.c              |   8 +
 security/smack/smack_lsm.c            |  24 +
 107 files changed, 1550 insertions(+), 1043 deletions(-)


base-commit: 38e316398e4e6338b80223fb5f74415c0513718f
-- 
2.34.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 11/30] selinux: implement set acl hook
  2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
@ 2022-09-26 14:08 ` Christian Brauner
  2022-09-27 22:55   ` Paul Moore
  2022-09-26 14:08 ` [PATCH v2 13/30] evm: " Christian Brauner
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2022-09-26 14:08 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Christian Brauner, Seth Forshee, Christoph Hellwig, Al Viro,
	linux-integrity, Paul Moore, Stephen Smalley, Eric Paris,
	selinux, linux-security-module

The current way of setting and getting posix acls through the generic
xattr interface is error prone and type unsafe. The vfs needs to
interpret and fixup posix acls before storing or reporting it to
userspace. Various hacks exist to make this work. The code is hard to
understand and difficult to maintain in it's current form. Instead of
making this work by hacking posix acls through xattr handlers we are
building a dedicated posix acl api around the get and set inode
operations. This removes a lot of hackiness and makes the codepaths
easier to maintain. A lot of background can be found in [1].

So far posix acls were passed as a void blob to the security and
integrity modules. Some of them like evm then proceed to interpret the
void pointer and convert it into the kernel internal struct posix acl
representation to perform their integrity checking magic. This is
obviously pretty problematic as that requires knowledge that only the
vfs is guaranteed to have and has lead to various bugs. Add a proper
security hook for setting posix acls and pass down the posix acls in
their appropriate vfs format instead of hacking it through a void
pointer stored in the uapi format.

I spent considerate time in the security module infrastructure and
audited all codepaths. SELinux has no restrictions based on the posix
acl values passed through it. The capability hook doesn't need to be
called either because it only has restrictions on security.* xattrs. So
this all becomes a very simple hook for SELinux.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Notes:
    /* v2 */
    unchanged

 security/selinux/hooks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79573504783b..bbc0ce3bde35 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3239,6 +3239,13 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns,
 			    &ad);
 }
 
+static int selinux_inode_set_acl(struct user_namespace *mnt_userns,
+				 struct dentry *dentry, const char *acl_name,
+				 struct posix_acl *kacl)
+{
+	return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+}
+
 static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 					const void *value, size_t size,
 					int flags)
@@ -7063,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
 	LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr),
 	LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
+	LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl),
 	LSM_HOOK_INIT(inode_getsecurity, selinux_inode_getsecurity),
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 13/30] evm: implement set acl hook
  2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
  2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
@ 2022-09-26 14:08 ` Christian Brauner
  2022-09-27 22:56   ` Paul Moore
  2022-09-26 14:08 ` [PATCH v2 15/30] evm: add post " Christian Brauner
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2022-09-26 14:08 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Christian Brauner, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

The current way of setting and getting posix acls through the generic
xattr interface is error prone and type unsafe. The vfs needs to
interpret and fixup posix acls before storing or reporting it to
userspace. Various hacks exist to make this work. The code is hard to
understand and difficult to maintain in it's current form. Instead of
making this work by hacking posix acls through xattr handlers we are
building a dedicated posix acl api around the get and set inode
operations. This removes a lot of hackiness and makes the codepaths
easier to maintain. A lot of background can be found in [1].

So far posix acls were passed as a void blob to the security and
integrity modules. Some of them like evm then proceed to interpret the
void pointer and convert it into the kernel internal struct posix acl
representation to perform their integrity checking magic. This is
obviously pretty problematic as that requires knowledge that only the
vfs is guaranteed to have and has lead to various bugs. Add a proper
security hook for setting posix acls and pass down the posix acls in
their appropriate vfs format instead of hacking it through a void
pointer stored in the uapi format.

I spent considerate time in the security module and integrity
infrastructure and audited all codepaths. EVM is the only part that
really has restrictions based on the actual posix acl values passed
through it. Before this dedicated hook EVM used to translate from the
uapi posix acl format sent to it in the form of a void pointer into the
vfs format. This is not a good thing. Instead of hacking around in the
uapi struct give EVM the posix acls in the appropriate vfs format and
perform sane permissions checks that mirror what it used to to in the
generic xattr hook.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Notes:
    /* v2 */
    unchanged

 include/linux/evm.h               | 10 +++++
 security/integrity/evm/evm_main.c | 66 ++++++++++++++++++++++++++++++-
 security/security.c               |  9 ++++-
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index aa63e0b3c0a2..aebcfd47d496 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -35,6 +35,9 @@ extern int evm_inode_removexattr(struct user_namespace *mnt_userns,
 				 struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
 				       const char *xattr_name);
+extern int evm_inode_set_acl(struct user_namespace *mnt_userns,
+			     struct dentry *dentry, const char *acl_name,
+			     struct posix_acl *kacl);
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -108,6 +111,13 @@ static inline void evm_inode_post_removexattr(struct dentry *dentry,
 	return;
 }
 
+static inline int evm_inode_set_acl(struct user_namespace *mnt_userns,
+				    struct dentry *dentry, const char *acl_name,
+				    struct posix_acl *kacl)
+{
+	return 0;
+}
+
 static inline int evm_inode_init_security(struct inode *inode,
 					  const struct xattr *xattr_array,
 					  struct xattr *evm)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 23d484e05e6f..15aa5995fff4 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -8,7 +8,7 @@
  *
  * File: evm_main.c
  *	implements evm_inode_setxattr, evm_inode_post_setxattr,
- *	evm_inode_removexattr, and evm_verifyxattr
+ *	evm_inode_removexattr, evm_verifyxattr, and evm_inode_set_acl.
  */
 
 #define pr_fmt(fmt) "EVM: "fmt
@@ -670,6 +670,70 @@ int evm_inode_removexattr(struct user_namespace *mnt_userns,
 	return evm_protect_xattr(mnt_userns, dentry, xattr_name, NULL, 0);
 }
 
+static int evm_inode_set_acl_change(struct user_namespace *mnt_userns,
+				    struct dentry *dentry, const char *name,
+				    struct posix_acl *kacl)
+{
+#ifdef CONFIG_FS_POSIX_ACL
+	int rc;
+	umode_t mode;
+	struct inode *inode = d_backing_inode(dentry);
+
+	rc = posix_acl_update_mode(mnt_userns, inode, &mode, &kacl);
+	if (rc || (inode->i_mode != mode))
+		return 1;
+#endif
+	return 0;
+}
+
+/**
+ * evm_inode_set_acl - protect the EVM extended attribute for posix acls
+ * @mnt_userns: user namespace of the idmapped mount
+ * @dentry: pointer to the affected dentry
+ * @acl_name: name of the posix acl
+ * @kacl: pointer to the posix acls
+ */
+int evm_inode_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
+		      const char *acl_name, struct posix_acl *kacl)
+{
+	enum integrity_status evm_status;
+
+	/* Policy permits modification of the protected xattrs even though
+	 * there's no HMAC key loaded
+	 */
+	if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+		return 0;
+
+	evm_status = evm_verify_current_integrity(dentry);
+	if ((evm_status == INTEGRITY_PASS) ||
+	    (evm_status == INTEGRITY_NOXATTRS))
+		return 0;
+
+	/* Exception if the HMAC is not going to be calculated. */
+	if (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL ||
+	    evm_status == INTEGRITY_UNKNOWN))
+		return 0;
+
+	/*
+	 * Writing other xattrs is safe for portable signatures, as portable
+	 * signatures are immutable and can never be updated.
+	 */
+	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
+		return 0;
+
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_inode_set_acl_change(mnt_userns, dentry, acl_name, kacl))
+		return 0;
+
+	if (evm_status != INTEGRITY_PASS &&
+	    evm_status != INTEGRITY_PASS_IMMUTABLE)
+		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
+				    dentry->d_name.name, "appraise_metadata",
+				    integrity_status_msg[evm_status],
+				    -EPERM, 0);
+	return evm_status == INTEGRITY_PASS ? 0 : -EPERM;
+}
+
 static void evm_reset_status(struct inode *inode)
 {
 	struct integrity_iint_cache *iint;
diff --git a/security/security.c b/security/security.c
index 56d48e7254d6..a12a26a4494e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1374,9 +1374,16 @@ int security_inode_set_acl(struct user_namespace *mnt_userns,
 			   struct dentry *dentry, const char *acl_name,
 			   struct posix_acl *kacl)
 {
+	int ret;
+
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	return call_int_hook(inode_set_acl, 0, mnt_userns, dentry, acl_name, kacl);
+
+	ret = call_int_hook(inode_set_acl, 0, mnt_userns, dentry, acl_name, kacl);
+	if (ret)
+		return ret;
+
+	return evm_inode_set_acl(mnt_userns, dentry, acl_name, kacl);
 }
 
 void security_inode_post_setxattr(struct dentry *dentry, const char *name,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 15/30] evm: add post set acl hook
  2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
  2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
  2022-09-26 14:08 ` [PATCH v2 13/30] evm: " Christian Brauner
@ 2022-09-26 14:08 ` Christian Brauner
  2022-09-27 22:56   ` Paul Moore
  2022-09-26 14:08 ` [PATCH v2 18/30] evm: simplify evm_xattr_acl_change() Christian Brauner
  2022-09-27  0:22 ` [PATCH v2 00/30] acl: add vfs posix acl api Casey Schaufler
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2022-09-26 14:08 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Christian Brauner, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

The security_inode_post_setxattr() hook is used by security modules to
update their own security.* xattrs. Consequently none of the security
modules operate on posix acls. So we don't need an additional security
hook when post setting posix acls.

However, the integrity subsystem wants to be informed about posix acl
changes and specifically evm to update their hashes when the xattrs
change. The callchain for evm_inode_post_setxattr() is:

-> evm_inode_post_setxattr()
   -> evm_update_evmxattr()
      -> evm_calc_hmac()
         -> evm_calc_hmac_or_hash()

and evm_cacl_hmac_or_hash() walks the global list of protected xattr
names evm_config_xattrnames. This global list can be modified via
/sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is
restricted to security.* xattrs and the default xattrs in
evm_config_xattrnames only contains security.* xattrs as well.

So the actual value for posix acls is currently completely irrelevant
for evm during evm_inode_post_setxattr() and frankly it should stay that
way in the future to not cause the vfs any more headaches. But if the
actual posix acl values matter then evm shouldn't operate on the binary
void blob and try to hack around in the uapi struct anyway. Instead it
should then in the future add a dedicated hook which takes a struct
posix_acl argument passing the posix acls in the proper vfs format.

For now it is sufficient to make evm_inode_post_set_acl() a wrapper
around evm_inode_post_setxattr() not passing any actual values down.
This will still cause the hashes to be updated as before.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Notes:
    /* v2 */
    unchanged

 fs/posix_acl.c      |  5 ++++-
 include/linux/evm.h | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 471d17fa1611..ef0908a4bc46 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -25,6 +25,7 @@
 #include <linux/namei.h>
 #include <linux/mnt_idmapping.h>
 #include <linux/security.h>
+#include <linux/evm.h>
 #include <linux/fsnotify.h>
 
 static struct posix_acl **acl_by_type(struct inode *inode, int type)
@@ -1351,8 +1352,10 @@ int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
 		error = -EIO;
 	else
 		error = -EOPNOTSUPP;
-	if (!error)
+	if (!error) {
 		fsnotify_xattr(dentry);
+		evm_inode_post_set_acl(dentry, acl_name, kacl);
+	}
 
 out_inode_unlock:
 	inode_unlock(inode);
diff --git a/include/linux/evm.h b/include/linux/evm.h
index aebcfd47d496..7811ce56e02f 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -38,6 +38,12 @@ extern void evm_inode_post_removexattr(struct dentry *dentry,
 extern int evm_inode_set_acl(struct user_namespace *mnt_userns,
 			     struct dentry *dentry, const char *acl_name,
 			     struct posix_acl *kacl);
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name,
+					  struct posix_acl *kacl)
+{
+	return evm_inode_post_setxattr(dentry, acl_name, NULL, 0);
+}
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -118,6 +124,13 @@ static inline int evm_inode_set_acl(struct user_namespace *mnt_userns,
 	return 0;
 }
 
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name,
+					  struct posix_acl *kacl)
+{
+	return;
+}
+
 static inline int evm_inode_init_security(struct inode *inode,
 					  const struct xattr *xattr_array,
 					  struct xattr *evm)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 18/30] evm: simplify evm_xattr_acl_change()
  2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
                   ` (2 preceding siblings ...)
  2022-09-26 14:08 ` [PATCH v2 15/30] evm: add post " Christian Brauner
@ 2022-09-26 14:08 ` Christian Brauner
  2022-09-27 22:56   ` Paul Moore
  2022-09-27  0:22 ` [PATCH v2 00/30] acl: add vfs posix acl api Casey Schaufler
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2022-09-26 14:08 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Christian Brauner, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

The posix acl api provides a dedicated security and integrity hook for
setting posix acls. This means that

evm_protect_xattr()
-> evm_xattr_change()
   -> evm_xattr_acl_change()

is now only hit during vfs_remove_acl() at which point we are guaranteed
that xattr_value and xattr_value_len are NULL and 0. In this case evm
always used to return 1. Simplify this function to do just that.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Notes:
    /* v2 */
    unchanged

 security/integrity/evm/evm_main.c | 62 +++++++------------------------
 1 file changed, 14 insertions(+), 48 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 15aa5995fff4..1fbe1b8d0364 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 
 /*
  * evm_xattr_acl_change - check if passed ACL changes the inode mode
- * @mnt_userns: user namespace of the idmapped mount
- * @dentry: pointer to the affected dentry
  * @xattr_name: requested xattr
  * @xattr_value: requested xattr value
  * @xattr_value_len: requested xattr value length
  *
- * Check if passed ACL changes the inode mode, which is protected by EVM.
+ * This is only hit during xattr removal at which point we always return 1.
+ * Splat a warning in case someone managed to pass data to this function. That
+ * should never happen.
  *
  * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
  */
-static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
-				struct dentry *dentry, const char *xattr_name,
-				const void *xattr_value, size_t xattr_value_len)
+static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
 {
-#ifdef CONFIG_FS_POSIX_ACL
-	umode_t mode;
-	struct posix_acl *acl = NULL, *acl_res;
-	struct inode *inode = d_backing_inode(dentry);
-	int rc;
-
-	/*
-	 * An earlier comment here mentioned that the idmappings for
-	 * ACL_{GROUP,USER} don't matter since EVM is only interested in the
-	 * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
-	 * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
-	 * representation it should do so correctly. There's no guarantee that
-	 * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
-	 * for the mode at some point and it's difficult to keep track of all
-	 * the LSM and integrity modules and what they do to POSIX ACLs.
-	 *
-	 * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
-	 * ACLs it received. It requires knowledge that only the VFS is
-	 * guaranteed to have.
-	 */
-	acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
-				  xattr_value, xattr_value_len);
-	if (IS_ERR_OR_NULL(acl))
-		return 1;
-
-	acl_res = acl;
-	/*
-	 * Passing mnt_userns is necessary to correctly determine the GID in
-	 * an idmapped mount, as the GID is used to clear the setgid bit in
-	 * the inode mode.
-	 */
-	rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
-
-	posix_acl_release(acl);
-
-	if (rc)
-		return 1;
+	int rc = 0;
 
-	if (inode->i_mode != mode)
-		return 1;
+#ifdef CONFIG_FS_POSIX_ACL
+	WARN_ONCE(xattr_value != NULL,
+		  "Passing xattr value for POSIX ACLs not supported\n");
+	WARN_ONCE(xattr_value_len != 0,
+		  "Passing non-zero length for POSIX ACLs not supported\n");
+	rc = 1;
 #endif
-	return 0;
+
+	return rc;
 }
 
 /*
@@ -514,8 +481,7 @@ static int evm_xattr_change(struct user_namespace *mnt_userns,
 	int rc = 0;
 
 	if (posix_xattr_acl(xattr_name))
-		return evm_xattr_acl_change(mnt_userns, dentry, xattr_name,
-					    xattr_value, xattr_value_len);
+		return evm_xattr_acl_change(xattr_value, xattr_value_len);
 
 	rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data,
 				0, GFP_NOFS);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
                   ` (3 preceding siblings ...)
  2022-09-26 14:08 ` [PATCH v2 18/30] evm: simplify evm_xattr_acl_change() Christian Brauner
@ 2022-09-27  0:22 ` Casey Schaufler
  2022-09-27  7:41   ` Christoph Hellwig
  4 siblings, 1 reply; 18+ messages in thread
From: Casey Schaufler @ 2022-09-27  0:22 UTC (permalink / raw)
  To: Christian Brauner, linux-fsdevel
  Cc: Seth Forshee, Christoph Hellwig, Linus Torvalds, Al Viro,
	v9fs-developer, linux-cifs, linux-integrity,
	linux-security-module, casey

On 9/26/2022 7:07 AM, Christian Brauner wrote:
> From: "Christian Brauner (Microsoft)" <brauner@kernel.org>
>
> Hey everyone,
>
> /* v2 */
> This fixes various things pointed out during review. The individual
> commits contain more details were appropriate.
>
> As we discussed and seen multiple times the current state of how posix
> acls are handled isn't nice and comes with a lot of problems. For a long
> and detailed explanation for just some of the issues [1] provides a good
> summary.
>
> The current way of handling posix acls via the generic xattr api is
> error prone, hard to maintain, and type unsafe for the vfs until we call
> into the filesystem's dedicated get and set inode operations.
>
> It is already the case that posix acls are special-cased to death all
> the way through the vfs. There are an uncounted number of hacks that
> operate on the uapi posix acl struct instead of the dedicated vfs struct
> posix_acl. And the vfs must be involved in order to interpret and fixup
> posix acls before storing them to the backing store, caching them,
> reporting them to userspace, or for permission checking.
>
> Currently a range of hacks and duct tape exist to make this work. As
> with most things this is really no ones fault it's just something that
> happened over time. But the code is hard to understand and difficult
> to maintain and one is constantly at risk of introducing bugs and
> regressions when having to touch it.
>
> Instead of continuing to hack posix acls through the xattr handlers this
> series builds a dedicated posix acl api solely around the get and set
> inode operations. Going forward, the vfs_get_acl(), vfs_remove_acl(),
> and vfs_set_acl() helpers must be used in order to interact with posix
> acls. They operate directly on the vfs internal struct posix_acl instead
> of abusing the uapi posix acl struct as we currently do. In the end this
> removes all of the hackiness, makes the codepaths easier to maintain,
> and gets us type safety.

I may have missed something obvious, but from the LSM side the only place
where the ACL handling differs from general xattr handling in in EVM. This
seems like a lot of "clean-up" to address that one special case. Especially
since EVM isn't (currently) an LSM. I'll admit that a future LSM might look
into the posix acl, although looking at another security feature's data is
hardly encouraged. EVM, which is all about protecting the content of other
security data is kind of exceptional in that.

I suggest that you might focus on the acl/evm interface rather than the entire
LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
I don't see how the breadth of this patch set is appropriate.

>
> This series passes the LTP and xfstests suites without any regressions.
> For xfstests the following combinations were tested:
>
> * xfs
> * ext4
> * btrfs
> * overlayfs
> * overlayfs on top of idmapped mounts
>
> For people wanting to run their own xfstests I'd recommend to shorten
> their test runs via:
>
> ./check -g acl,attr,cap,idmapped,io_uring,perms,subvol,unlink
>
> I would appreciate if the 9p and cifs folks could run any posix acl
> related tests as I have no setup to really do this without causing me a
> lot of pain.
>
> Very likely there's a lot more simplifications for posix acls that we
> can make in the future if the basic api has made it.
>
> A few implementation details:
>
> * The series makes sure to retain exactly the same security and
>   integrity module permission checks. See [2] for annotated callchains.
>   Especially for the integrity modules this api is a win because right
>   now they convert the uapi posix acl struct passed to them via a void
>   pointer into the vfs struct posix_acl format to perform permission
>   checking on the mode.
>
>   There's a new dedicated security hook for setting posix acls which
>   passes the vfs struct posix_acl not a void pointer. Basing checking on
>   the posix acl stored in the uapi format is really unreliable. The vfs
>   currently hacks around directly in the uapi struct storing values that
>   frankly the security and integrity modules can't correctly interpret
>   as evidenced by bugs we reported and fixed in this area. It's not
>   necessarily even their fault it's just that the format we provide to
>   them is sub optimal.
>
> * Some filesystems like 9p and cifs need access to the dentry in order
>   to get and set posix acls which is why they either only partially or
>   not even at all implement get and set inode operations. For example,
>   cifs allows setxattr() and getxattr() operations but doesn't allow
>   permission checking based on posix acls because it can't implement a
>   get acl inode operation.
>
>   Thus, this patch series updates the set acl inode operation to take a
>   dentry instead of an inode argument. However, for the get acl inode
>   operation we can't do this as the old get acl method is called in
>   e.g., generic_permission() and inode_permission(). These helpers in
>   turn are called in various filesystem's permission inode operation. So
>   passing a dentry argument to the old get acl inode operation would
>   amount to passing a dentry to the permission inode operation which we
>   shouldn't and probably can't do.
>
>   So instead of extending the existing inode operation Christoph
>   suggested to add a new one. He also requested to ensure that the get
>   and set acl inode operation taking a dentry are consistently named. So
>   for this version the old get acl operation is renamed to
>   ->get_inode_acl() and a new ->get_acl() inode operation taking a
>   dentry is added. With this we can give both 9p and cifs get and set
>   acl inode operations and in turn remove their complex custom posix
>   xattr handlers.
>
> * I've done a full audit of every codepaths using variant of the
>   current generic xattr api to get and set posix acls and surprisingly
>   it isn't that many places. There's of course always a chance that I
>   might have missed some and I'm sure we'll find them soon enough.
>
>   The crucial codepaths to be converted are obviously stacking
>   filesystems such as ecryptfs and overlayfs.
>
>   For a list of all callers currently using generic xattr api helpers
>   see [2] including comments whether they support posix acls or not.
>
> * The old vfs generic posix acl infrastructure doesn't obey
>   the create and replace semantics promised on the setxattr(2) manpage.
>   This patch series doesn't address this. It really is something we
>   should revisit later though.
>
> The patch series is roughly organized as follows:
>
> // intended to be a non-functional change
> 1. Change existing set acl inode operation to take a dentry argument.
>
> // intended to be a non-functional change
> 2. Rename existing get acl method.
>
> // intended to be a non-functional change
> 3. Implement get and set acl inode operations for filesystems that
>    couldn't implement one before because of the missing dentry. That's
>    mostly 9p and cifs.
>
> // intended to be a non-functional change
> 4. Build posix acl api, i.e., add vfs_get_acl(), vfs_remove_acl(), and
>    vfs_set_acl() including security and integrity hooks.
>
> // intended to be a non-functional change
> 5. Implement get and set acl inode operations for stacking filesystems.
>
> // semantical change
> 6. Switch posix acl handling in stacking filesystems to new posix acl
>    api now that all filesystems it can stack upon support it.
>
> // semantical change
> 7. Switch vfs to new posix acl api
>
> 8. Remove all now unused helpers
>
> The series can be pulled from:
>
> https://gitlab.com/brauner/linux/-/commits/fs.acl.rework
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
>
> The series contains a few preliminary patches which are scheduled for
> the next merge window. It was just easier to base the series on top of
> them. But if you pull this branch you'll get them included.
>
> I've been working on this for a while and before going any further it'd
> be nice to get some reviews. I think that it should be fine to have get
> and set acl inode operations that operate on the dentry at least nothing
> stuck out immediately that would prevent this. But obviously having
> other people point out issues with that would be helpful.
>
> Thanks to Seth for a lot of good discussion around this and
> encouragement and input from Christoph.
>
> [1]: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org
> [2]: https://gist.github.com/brauner/12c795b93a05dc3b3056b1982549a633
>
> v1: https://lore.kernel.org/linux-cifs/20220922151728.1557914-1-brauner@kernel.org
>
> Thanks!
> Christian
>
> Christian Brauner (30):
>   orangefs: rework posix acl handling when creating new filesystem
>     objects
>   fs: pass dentry to set acl method
>   fs: rename current get acl method
>   fs: add new get acl method
>   cifs: implement get acl method
>   cifs: implement set acl method
>   9p: implement get acl method
>   9p: implement set acl method
>   acl: add vfs_set_acl()
>   security: add set acl hook
>   selinux: implement set acl hook
>   smack: implement set acl hook
>   evm: implement set acl hook
>   acl: use set acl hook
>   evm: add post set acl hook
>   acl: add vfs_get_acl()
>   acl: add vfs_remove_acl()
>   evm: simplify evm_xattr_acl_change()
>   ksmbd: use vfs_remove_acl()
>   ecryptfs: implement get acl method
>   ecryptfs: implement set acl method
>   ovl: implement get acl method
>   ovl: implement set acl method
>   ovl: use posix acl api
>   xattr: use posix acl api
>   ecryptfs: use stub posix acl handlers
>   ovl: use stub posix acl handlers
>   cifs: use stub posix acl handlers
>   9p: use stub posix acl handlers
>   acl: remove a slew of now unused helpers
>
>  Documentation/filesystems/locking.rst |   4 +-
>  Documentation/filesystems/porting.rst |   4 +-
>  Documentation/filesystems/vfs.rst     |   3 +-
>  fs/9p/acl.c                           | 295 +++++------
>  fs/9p/acl.h                           |   8 +-
>  fs/9p/vfs_inode_dotl.c                |   4 +
>  fs/9p/xattr.c                         |   7 +-
>  fs/9p/xattr.h                         |   2 -
>  fs/bad_inode.c                        |   4 +-
>  fs/btrfs/acl.c                        |   3 +-
>  fs/btrfs/ctree.h                      |   2 +-
>  fs/btrfs/inode.c                      |   8 +-
>  fs/ceph/acl.c                         |   3 +-
>  fs/ceph/dir.c                         |   2 +-
>  fs/ceph/inode.c                       |   4 +-
>  fs/ceph/super.h                       |   2 +-
>  fs/cifs/cifsacl.c                     | 141 ++++++
>  fs/cifs/cifsfs.c                      |   4 +
>  fs/cifs/cifsproto.h                   |  20 +-
>  fs/cifs/cifssmb.c                     | 206 +++++---
>  fs/cifs/xattr.c                       |  68 +--
>  fs/ecryptfs/inode.c                   |  32 ++
>  fs/erofs/inode.c                      |   6 +-
>  fs/erofs/namei.c                      |   2 +-
>  fs/ext2/acl.c                         |   3 +-
>  fs/ext2/acl.h                         |   2 +-
>  fs/ext2/file.c                        |   2 +-
>  fs/ext2/inode.c                       |   2 +-
>  fs/ext2/namei.c                       |   4 +-
>  fs/ext4/acl.c                         |   3 +-
>  fs/ext4/acl.h                         |   2 +-
>  fs/ext4/file.c                        |   2 +-
>  fs/ext4/inode.c                       |   2 +-
>  fs/ext4/namei.c                       |   4 +-
>  fs/f2fs/acl.c                         |   4 +-
>  fs/f2fs/acl.h                         |   2 +-
>  fs/f2fs/file.c                        |   4 +-
>  fs/f2fs/namei.c                       |   4 +-
>  fs/fuse/acl.c                         |   3 +-
>  fs/fuse/dir.c                         |   4 +-
>  fs/fuse/fuse_i.h                      |   2 +-
>  fs/gfs2/acl.c                         |   3 +-
>  fs/gfs2/acl.h                         |   2 +-
>  fs/gfs2/inode.c                       |   6 +-
>  fs/internal.h                         |   1 +
>  fs/jffs2/acl.c                        |   3 +-
>  fs/jffs2/acl.h                        |   2 +-
>  fs/jffs2/dir.c                        |   2 +-
>  fs/jffs2/file.c                       |   2 +-
>  fs/jffs2/fs.c                         |   2 +-
>  fs/jfs/acl.c                          |   3 +-
>  fs/jfs/file.c                         |   4 +-
>  fs/jfs/jfs_acl.h                      |   2 +-
>  fs/jfs/namei.c                        |   2 +-
>  fs/ksmbd/smb2pdu.c                    |   4 +-
>  fs/ksmbd/smbacl.c                     |   4 +-
>  fs/ksmbd/vfs.c                        |  17 +-
>  fs/ksmbd/vfs.h                        |   4 +-
>  fs/namei.c                            |   2 +-
>  fs/nfs/nfs3_fs.h                      |   2 +-
>  fs/nfs/nfs3acl.c                      |   3 +-
>  fs/nfs/nfs3proc.c                     |   4 +-
>  fs/nfsd/nfs2acl.c                     |   4 +-
>  fs/nfsd/nfs3acl.c                     |   4 +-
>  fs/nfsd/vfs.c                         |   4 +-
>  fs/ntfs3/file.c                       |   4 +-
>  fs/ntfs3/namei.c                      |   4 +-
>  fs/ntfs3/ntfs_fs.h                    |   4 +-
>  fs/ntfs3/xattr.c                      |   9 +-
>  fs/ocfs2/acl.c                        |   3 +-
>  fs/ocfs2/acl.h                        |   2 +-
>  fs/ocfs2/file.c                       |   4 +-
>  fs/ocfs2/namei.c                      |   2 +-
>  fs/orangefs/acl.c                     |  47 +-
>  fs/orangefs/inode.c                   |  47 +-
>  fs/orangefs/namei.c                   |   2 +-
>  fs/orangefs/orangefs-kernel.h         |   9 +-
>  fs/orangefs/orangefs-utils.c          |  12 +-
>  fs/overlayfs/copy_up.c                |  38 ++
>  fs/overlayfs/dir.c                    |  22 +-
>  fs/overlayfs/inode.c                  | 151 +++++-
>  fs/overlayfs/overlayfs.h              |  34 +-
>  fs/overlayfs/super.c                  | 107 +---
>  fs/posix_acl.c                        | 681 +++++++++++++-------------
>  fs/reiserfs/acl.h                     |   6 +-
>  fs/reiserfs/file.c                    |   2 +-
>  fs/reiserfs/inode.c                   |   2 +-
>  fs/reiserfs/namei.c                   |   4 +-
>  fs/reiserfs/xattr_acl.c               |   9 +-
>  fs/xattr.c                            |  78 ++-
>  fs/xfs/xfs_acl.c                      |   3 +-
>  fs/xfs/xfs_acl.h                      |   2 +-
>  fs/xfs/xfs_iops.c                     |  16 +-
>  include/linux/evm.h                   |  23 +
>  include/linux/fs.h                    |  10 +-
>  include/linux/lsm_hook_defs.h         |   2 +
>  include/linux/lsm_hooks.h             |   4 +
>  include/linux/posix_acl.h             |  39 +-
>  include/linux/posix_acl_xattr.h       |  43 +-
>  include/linux/security.h              |  11 +
>  include/linux/xattr.h                 |   8 +
>  io_uring/xattr.c                      |   2 +
>  mm/shmem.c                            |   2 +-
>  security/integrity/evm/evm_main.c     | 128 +++--
>  security/security.c                   |  16 +
>  security/selinux/hooks.c              |   8 +
>  security/smack/smack_lsm.c            |  24 +
>  107 files changed, 1550 insertions(+), 1043 deletions(-)
>
>
> base-commit: 38e316398e4e6338b80223fb5f74415c0513718f

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27  0:22 ` [PATCH v2 00/30] acl: add vfs posix acl api Casey Schaufler
@ 2022-09-27  7:41   ` Christoph Hellwig
  2022-09-27  7:59     ` Christian Brauner
  2022-09-27 14:11     ` Casey Schaufler
  0 siblings, 2 replies; 18+ messages in thread
From: Christoph Hellwig @ 2022-09-27  7:41 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Christian Brauner, linux-fsdevel, Seth Forshee,
	Christoph Hellwig, Linus Torvalds, Al Viro, v9fs-developer,
	linux-cifs, linux-integrity, linux-security-module

On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
> I suggest that you might focus on the acl/evm interface rather than the entire
> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
> I don't see how the breadth of this patch set is appropriate.

Umm. The problem is the historically the Linux xattr interface was
intended for unstructured data, while some of it is very much structured
and requires interpretation by the VFS and associated entities.  So
splitting these out and add proper interface is absolutely the right
thing to do and long overdue (also for other thing like capabilities).
It might make things a little more verbose for LSM, but it fixes a very
real problem.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27  7:41   ` Christoph Hellwig
@ 2022-09-27  7:59     ` Christian Brauner
  2022-09-27 14:11     ` Casey Schaufler
  1 sibling, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2022-09-27  7:59 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Casey Schaufler, linux-fsdevel, Seth Forshee, Linus Torvalds,
	Al Viro, v9fs-developer, linux-cifs, linux-integrity,
	linux-security-module

On Tue, Sep 27, 2022 at 09:41:01AM +0200, Christoph Hellwig wrote:
> On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
> > I suggest that you might focus on the acl/evm interface rather than the entire
> > LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
> > I don't see how the breadth of this patch set is appropriate.
> 
> Umm. The problem is the historically the Linux xattr interface was
> intended for unstructured data, while some of it is very much structured
> and requires interpretation by the VFS and associated entities.  So
> splitting these out and add proper interface is absolutely the right
> thing to do and long overdue (also for other thing like capabilities).
> It might make things a little more verbose for LSM, but it fixes a very
> real problem.

Agreed.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27  7:41   ` Christoph Hellwig
  2022-09-27  7:59     ` Christian Brauner
@ 2022-09-27 14:11     ` Casey Schaufler
  2022-09-27 15:16       ` Seth Forshee
  2022-09-27 23:24       ` Paul Moore
  1 sibling, 2 replies; 18+ messages in thread
From: Casey Schaufler @ 2022-09-27 14:11 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Christian Brauner, linux-fsdevel, Seth Forshee, Linus Torvalds,
	Al Viro, v9fs-developer, linux-cifs, linux-integrity,
	linux-security-module, casey

On 9/27/2022 12:41 AM, Christoph Hellwig wrote:
> On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
>> I suggest that you might focus on the acl/evm interface rather than the entire
>> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
>> I don't see how the breadth of this patch set is appropriate.
> Umm. The problem is the historically the Linux xattr interface was
> intended for unstructured data, while some of it is very much structured
> and requires interpretation by the VFS and associated entities.  So
> splitting these out and add proper interface is absolutely the right
> thing to do and long overdue (also for other thing like capabilities).
> It might make things a little more verbose for LSM, but it fixes a very
> real problem.

Here's the problem I see. All of the LSMs see xattrs, except for their own,
as opaque objects. Introducing LSM hooks to address the data interpretation
issues between VFS and EVM, which is not an LSM, adds to an already overlarge
and interface. And the "real" users of the interface don't need the new hook.
I'm not saying that the ACL doesn't have problems. I'm not saying that the
solution you've proposed isn't better than what's there now. I am saying that
using LSM as a conduit between VFS and EVM at the expense of the rest of the
modules is dubious. A lot of change to LSM for no value to LSM.

I am not adamant about this. A whole lot worse has been done for worse reasons.
But as Paul says, we're overdue to make an effort to keep the LSM interface sane.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27 14:11     ` Casey Schaufler
@ 2022-09-27 15:16       ` Seth Forshee
  2022-09-27 15:55         ` Casey Schaufler
  2022-09-27 23:24       ` Paul Moore
  1 sibling, 1 reply; 18+ messages in thread
From: Seth Forshee @ 2022-09-27 15:16 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Christoph Hellwig, Christian Brauner, linux-fsdevel,
	Linus Torvalds, Al Viro, v9fs-developer, linux-cifs,
	linux-integrity, linux-security-module

On Tue, Sep 27, 2022 at 07:11:17AM -0700, Casey Schaufler wrote:
> On 9/27/2022 12:41 AM, Christoph Hellwig wrote:
> > On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
> >> I suggest that you might focus on the acl/evm interface rather than the entire
> >> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
> >> I don't see how the breadth of this patch set is appropriate.
> > Umm. The problem is the historically the Linux xattr interface was
> > intended for unstructured data, while some of it is very much structured
> > and requires interpretation by the VFS and associated entities.  So
> > splitting these out and add proper interface is absolutely the right
> > thing to do and long overdue (also for other thing like capabilities).
> > It might make things a little more verbose for LSM, but it fixes a very
> > real problem.
> 
> Here's the problem I see. All of the LSMs see xattrs, except for their own,
> as opaque objects. Introducing LSM hooks to address the data interpretation
> issues between VFS and EVM, which is not an LSM, adds to an already overlarge
> and interface. And the "real" users of the interface don't need the new hook.
> I'm not saying that the ACL doesn't have problems. I'm not saying that the
> solution you've proposed isn't better than what's there now. I am saying that
> using LSM as a conduit between VFS and EVM at the expense of the rest of the
> modules is dubious. A lot of change to LSM for no value to LSM.
> 
> I am not adamant about this. A whole lot worse has been done for worse reasons.
> But as Paul says, we're overdue to make an effort to keep the LSM interface sane.

So I assume the alternative you have in mind would be to use the
existing setxattr hook? I worry about type confusion if an LSM does
someday want to look inside the ACL data. Unless LSMs aren't supposed to
look inside of xattr data, but in that case why pass the data pointer on
to the LSMs?

Note that the caller of this new hook does not have access to the uapi
xattr data, and I think this is the right place for the new hook to be
called as it's the interface that stacked filesystems like overlayfs
will use to write ACLs to the lower filesystems.

Seth

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27 15:16       ` Seth Forshee
@ 2022-09-27 15:55         ` Casey Schaufler
  0 siblings, 0 replies; 18+ messages in thread
From: Casey Schaufler @ 2022-09-27 15:55 UTC (permalink / raw)
  To: Seth Forshee
  Cc: Christoph Hellwig, Christian Brauner, linux-fsdevel,
	Linus Torvalds, Al Viro, v9fs-developer, linux-cifs,
	linux-integrity, linux-security-module, casey

On 9/27/2022 8:16 AM, Seth Forshee wrote:
> On Tue, Sep 27, 2022 at 07:11:17AM -0700, Casey Schaufler wrote:
>> On 9/27/2022 12:41 AM, Christoph Hellwig wrote:
>>> On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
>>>> I suggest that you might focus on the acl/evm interface rather than the entire
>>>> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
>>>> I don't see how the breadth of this patch set is appropriate.
>>> Umm. The problem is the historically the Linux xattr interface was
>>> intended for unstructured data, while some of it is very much structured
>>> and requires interpretation by the VFS and associated entities.  So
>>> splitting these out and add proper interface is absolutely the right
>>> thing to do and long overdue (also for other thing like capabilities).
>>> It might make things a little more verbose for LSM, but it fixes a very
>>> real problem.
>> Here's the problem I see. All of the LSMs see xattrs, except for their own,
>> as opaque objects. Introducing LSM hooks to address the data interpretation
>> issues between VFS and EVM, which is not an LSM, adds to an already overlarge
>> and interface. And the "real" users of the interface don't need the new hook.
>> I'm not saying that the ACL doesn't have problems. I'm not saying that the
>> solution you've proposed isn't better than what's there now. I am saying that
>> using LSM as a conduit between VFS and EVM at the expense of the rest of the
>> modules is dubious. A lot of change to LSM for no value to LSM.
>>
>> I am not adamant about this. A whole lot worse has been done for worse reasons.
>> But as Paul says, we're overdue to make an effort to keep the LSM interface sane.
> So I assume the alternative you have in mind would be to use the
> existing setxattr hook?

That is how it works today.

>  I worry about type confusion if an LSM does
> someday want to look inside the ACL data.

I suggest that changes to system behavior based on the content of
an ACL really belongs in the ACL code, not in an LSM. Can I imagine
someone wanting to add SELinux policy that controls what entries
are allowed to be set by a particular domain? Sure, but I can't see
how that would be popular with existing ACL fans.

>  Unless LSMs aren't supposed to
> look inside of xattr data, but in that case why pass the data pointer on
> to the LSMs?

So that the LSM can look at it's own xattr data.

> Note that the caller of this new hook does not have access to the uapi
> xattr data, and I think this is the right place for the new hook to be
> called as it's the interface that stacked filesystems like overlayfs
> will use to write ACLs to the lower filesystems.

I'm not saying anything about the organization of the calling code.
Why is it calling

	security_acl_hooha(...)

instead of

	evm_acl_hooha(...)


>
> Seth

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 11/30] selinux: implement set acl hook
  2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
@ 2022-09-27 22:55   ` Paul Moore
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2022-09-27 22:55 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-fsdevel, Seth Forshee, Christoph Hellwig, Al Viro,
	linux-integrity, Stephen Smalley, Eric Paris, selinux,
	linux-security-module

On Mon, Sep 26, 2022 at 10:09 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The current way of setting and getting posix acls through the generic
> xattr interface is error prone and type unsafe. The vfs needs to
> interpret and fixup posix acls before storing or reporting it to
> userspace. Various hacks exist to make this work. The code is hard to
> understand and difficult to maintain in it's current form. Instead of
> making this work by hacking posix acls through xattr handlers we are
> building a dedicated posix acl api around the get and set inode
> operations. This removes a lot of hackiness and makes the codepaths
> easier to maintain. A lot of background can be found in [1].
>
> So far posix acls were passed as a void blob to the security and
> integrity modules. Some of them like evm then proceed to interpret the
> void pointer and convert it into the kernel internal struct posix acl
> representation to perform their integrity checking magic. This is
> obviously pretty problematic as that requires knowledge that only the
> vfs is guaranteed to have and has lead to various bugs. Add a proper
> security hook for setting posix acls and pass down the posix acls in
> their appropriate vfs format instead of hacking it through a void
> pointer stored in the uapi format.
>
> I spent considerate time in the security module infrastructure and
> audited all codepaths. SELinux has no restrictions based on the posix
> acl values passed through it. The capability hook doesn't need to be
> called either because it only has restrictions on security.* xattrs. So
> this all becomes a very simple hook for SELinux.
>
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
>     /* v2 */
>     unchanged
>
>  security/selinux/hooks.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Acked-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 13/30] evm: implement set acl hook
  2022-09-26 14:08 ` [PATCH v2 13/30] evm: " Christian Brauner
@ 2022-09-27 22:56   ` Paul Moore
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2022-09-27 22:56 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-fsdevel, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The current way of setting and getting posix acls through the generic
> xattr interface is error prone and type unsafe. The vfs needs to
> interpret and fixup posix acls before storing or reporting it to
> userspace. Various hacks exist to make this work. The code is hard to
> understand and difficult to maintain in it's current form. Instead of
> making this work by hacking posix acls through xattr handlers we are
> building a dedicated posix acl api around the get and set inode
> operations. This removes a lot of hackiness and makes the codepaths
> easier to maintain. A lot of background can be found in [1].
>
> So far posix acls were passed as a void blob to the security and
> integrity modules. Some of them like evm then proceed to interpret the
> void pointer and convert it into the kernel internal struct posix acl
> representation to perform their integrity checking magic. This is
> obviously pretty problematic as that requires knowledge that only the
> vfs is guaranteed to have and has lead to various bugs. Add a proper
> security hook for setting posix acls and pass down the posix acls in
> their appropriate vfs format instead of hacking it through a void
> pointer stored in the uapi format.
>
> I spent considerate time in the security module and integrity
> infrastructure and audited all codepaths. EVM is the only part that
> really has restrictions based on the actual posix acl values passed
> through it. Before this dedicated hook EVM used to translate from the
> uapi posix acl format sent to it in the form of a void pointer into the
> vfs format. This is not a good thing. Instead of hacking around in the
> uapi struct give EVM the posix acls in the appropriate vfs format and
> perform sane permissions checks that mirror what it used to to in the
> generic xattr hook.
>
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
>     /* v2 */
>     unchanged
>
>  include/linux/evm.h               | 10 +++++
>  security/integrity/evm/evm_main.c | 66 ++++++++++++++++++++++++++++++-
>  security/security.c               |  9 ++++-
>  3 files changed, 83 insertions(+), 2 deletions(-)

Ultimately this is Mimi's call, and it can be done later after this
patchset is merged, but it seems to me that some of the code in
evm_inode_set_acl() could be pulled out into a helper function(s)
shared with evm_protect_xattr().

Reviewed-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 15/30] evm: add post set acl hook
  2022-09-26 14:08 ` [PATCH v2 15/30] evm: add post " Christian Brauner
@ 2022-09-27 22:56   ` Paul Moore
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2022-09-27 22:56 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-fsdevel, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The security_inode_post_setxattr() hook is used by security modules to
> update their own security.* xattrs. Consequently none of the security
> modules operate on posix acls. So we don't need an additional security
> hook when post setting posix acls.
>
> However, the integrity subsystem wants to be informed about posix acl
> changes and specifically evm to update their hashes when the xattrs
> change. The callchain for evm_inode_post_setxattr() is:
>
> -> evm_inode_post_setxattr()
>    -> evm_update_evmxattr()
>       -> evm_calc_hmac()
>          -> evm_calc_hmac_or_hash()
>
> and evm_cacl_hmac_or_hash() walks the global list of protected xattr
> names evm_config_xattrnames. This global list can be modified via
> /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is
> restricted to security.* xattrs and the default xattrs in
> evm_config_xattrnames only contains security.* xattrs as well.
>
> So the actual value for posix acls is currently completely irrelevant
> for evm during evm_inode_post_setxattr() and frankly it should stay that
> way in the future to not cause the vfs any more headaches. But if the
> actual posix acl values matter then evm shouldn't operate on the binary
> void blob and try to hack around in the uapi struct anyway. Instead it
> should then in the future add a dedicated hook which takes a struct
> posix_acl argument passing the posix acls in the proper vfs format.
>
> For now it is sufficient to make evm_inode_post_set_acl() a wrapper
> around evm_inode_post_setxattr() not passing any actual values down.
> This will still cause the hashes to be updated as before.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
>     /* v2 */
>     unchanged
>
>  fs/posix_acl.c      |  5 ++++-
>  include/linux/evm.h | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 18/30] evm: simplify evm_xattr_acl_change()
  2022-09-26 14:08 ` [PATCH v2 18/30] evm: simplify evm_xattr_acl_change() Christian Brauner
@ 2022-09-27 22:56   ` Paul Moore
  2022-09-28 13:31     ` Christian Brauner
  0 siblings, 1 reply; 18+ messages in thread
From: Paul Moore @ 2022-09-27 22:56 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-fsdevel, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The posix acl api provides a dedicated security and integrity hook for
> setting posix acls. This means that
>
> evm_protect_xattr()
> -> evm_xattr_change()
>    -> evm_xattr_acl_change()
>
> is now only hit during vfs_remove_acl() at which point we are guaranteed
> that xattr_value and xattr_value_len are NULL and 0. In this case evm
> always used to return 1. Simplify this function to do just that.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
>     /* v2 */
>     unchanged
>
>  security/integrity/evm/evm_main.c | 62 +++++++------------------------
>  1 file changed, 14 insertions(+), 48 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 15aa5995fff4..1fbe1b8d0364 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
>
>  /*
>   * evm_xattr_acl_change - check if passed ACL changes the inode mode
> - * @mnt_userns: user namespace of the idmapped mount
> - * @dentry: pointer to the affected dentry
>   * @xattr_name: requested xattr
>   * @xattr_value: requested xattr value
>   * @xattr_value_len: requested xattr value length
>   *
> - * Check if passed ACL changes the inode mode, which is protected by EVM.
> + * This is only hit during xattr removal at which point we always return 1.
> + * Splat a warning in case someone managed to pass data to this function. That
> + * should never happen.
>   *
>   * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
>   */
> -static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
> -                               struct dentry *dentry, const char *xattr_name,
> -                               const void *xattr_value, size_t xattr_value_len)
> +static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
>  {
> -#ifdef CONFIG_FS_POSIX_ACL
> -       umode_t mode;
> -       struct posix_acl *acl = NULL, *acl_res;
> -       struct inode *inode = d_backing_inode(dentry);
> -       int rc;
> -
> -       /*
> -        * An earlier comment here mentioned that the idmappings for
> -        * ACL_{GROUP,USER} don't matter since EVM is only interested in the
> -        * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
> -        * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
> -        * representation it should do so correctly. There's no guarantee that
> -        * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
> -        * for the mode at some point and it's difficult to keep track of all
> -        * the LSM and integrity modules and what they do to POSIX ACLs.
> -        *
> -        * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
> -        * ACLs it received. It requires knowledge that only the VFS is
> -        * guaranteed to have.
> -        */
> -       acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
> -                                 xattr_value, xattr_value_len);
> -       if (IS_ERR_OR_NULL(acl))
> -               return 1;
> -
> -       acl_res = acl;
> -       /*
> -        * Passing mnt_userns is necessary to correctly determine the GID in
> -        * an idmapped mount, as the GID is used to clear the setgid bit in
> -        * the inode mode.
> -        */
> -       rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
> -
> -       posix_acl_release(acl);
> -
> -       if (rc)
> -               return 1;
> +       int rc = 0;
>
> -       if (inode->i_mode != mode)
> -               return 1;
> +#ifdef CONFIG_FS_POSIX_ACL
> +       WARN_ONCE(xattr_value != NULL,
> +                 "Passing xattr value for POSIX ACLs not supported\n");
> +       WARN_ONCE(xattr_value_len != 0,
> +                 "Passing non-zero length for POSIX ACLs not supported\n");
> +       rc = 1;
>  #endif
> -       return 0;
> +
> +       return rc;
>  }

This is another case where I'll leave the final say up to Mimi, but
why not just get rid of evm_xattr_acl_change() entirely?  Unless I'm
missing something, it's only reason for existing now is to check that
it is passed the proper (empty) parameters which seems pointless ...
no?

--
paul-moore.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27 14:11     ` Casey Schaufler
  2022-09-27 15:16       ` Seth Forshee
@ 2022-09-27 23:24       ` Paul Moore
  2022-09-27 23:37         ` Casey Schaufler
  1 sibling, 1 reply; 18+ messages in thread
From: Paul Moore @ 2022-09-27 23:24 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Christoph Hellwig, Christian Brauner, linux-fsdevel,
	Seth Forshee, Linus Torvalds, Al Viro, v9fs-developer,
	linux-cifs, linux-integrity, linux-security-module

On Tue, Sep 27, 2022 at 10:13 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 9/27/2022 12:41 AM, Christoph Hellwig wrote:
> > On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
> >> I suggest that you might focus on the acl/evm interface rather than the entire
> >> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
> >> I don't see how the breadth of this patch set is appropriate.
> > Umm. The problem is the historically the Linux xattr interface was
> > intended for unstructured data, while some of it is very much structured
> > and requires interpretation by the VFS and associated entities.  So
> > splitting these out and add proper interface is absolutely the right
> > thing to do and long overdue (also for other thing like capabilities).
> > It might make things a little more verbose for LSM, but it fixes a very
> > real problem.
>
> Here's the problem I see. All of the LSMs see xattrs, except for their own,
> as opaque objects. Introducing LSM hooks to address the data interpretation
> issues between VFS and EVM, which is not an LSM, adds to an already overlarge
> and interface. And the "real" users of the interface don't need the new hook.
> I'm not saying that the ACL doesn't have problems. I'm not saying that the
> solution you've proposed isn't better than what's there now. I am saying that
> using LSM as a conduit between VFS and EVM at the expense of the rest of the
> modules is dubious. A lot of change to LSM for no value to LSM.

Let's take a step back and look not just at the LSM changes, but the
patchset as a whole.  Forgive my paraphrasing, but what Christian is
trying to do here is introduce a proper ACL API in the kernel to
remove a lot of kludges, special-cases, etc. in the VFS layer,
enabling better type checking, code abstractions, and all the nice
things you get when you have nice APIs.  This is admirable work, even
if it does result in some duplication at the LSM layer (and below).

It is my opinion that the impact to the LSM, both at the LSM layer,
and in the individual affected LSMs is not significant enough to
outweigh the other advantages offered by this patchset.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 00/30] acl: add vfs posix acl api
  2022-09-27 23:24       ` Paul Moore
@ 2022-09-27 23:37         ` Casey Schaufler
  0 siblings, 0 replies; 18+ messages in thread
From: Casey Schaufler @ 2022-09-27 23:37 UTC (permalink / raw)
  To: Paul Moore
  Cc: Christoph Hellwig, Christian Brauner, linux-fsdevel,
	Seth Forshee, Linus Torvalds, Al Viro, v9fs-developer,
	linux-cifs, linux-integrity, linux-security-module, casey

On 9/27/2022 4:24 PM, Paul Moore wrote:
> On Tue, Sep 27, 2022 at 10:13 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/27/2022 12:41 AM, Christoph Hellwig wrote:
>>> On Mon, Sep 26, 2022 at 05:22:45PM -0700, Casey Schaufler wrote:
>>>> I suggest that you might focus on the acl/evm interface rather than the entire
>>>> LSM interface. Unless there's a serious plan to make ima/evm into a proper LSM
>>>> I don't see how the breadth of this patch set is appropriate.
>>> Umm. The problem is the historically the Linux xattr interface was
>>> intended for unstructured data, while some of it is very much structured
>>> and requires interpretation by the VFS and associated entities.  So
>>> splitting these out and add proper interface is absolutely the right
>>> thing to do and long overdue (also for other thing like capabilities).
>>> It might make things a little more verbose for LSM, but it fixes a very
>>> real problem.
>> Here's the problem I see. All of the LSMs see xattrs, except for their own,
>> as opaque objects. Introducing LSM hooks to address the data interpretation
>> issues between VFS and EVM, which is not an LSM, adds to an already overlarge
>> and interface. And the "real" users of the interface don't need the new hook.
>> I'm not saying that the ACL doesn't have problems. I'm not saying that the
>> solution you've proposed isn't better than what's there now. I am saying that
>> using LSM as a conduit between VFS and EVM at the expense of the rest of the
>> modules is dubious. A lot of change to LSM for no value to LSM.
> Let's take a step back and look not just at the LSM changes, but the
> patchset as a whole.  Forgive my paraphrasing, but what Christian is
> trying to do here is introduce a proper ACL API in the kernel to
> remove a lot of kludges, special-cases, etc. in the VFS layer,
> enabling better type checking, code abstractions, and all the nice
> things you get when you have nice APIs.  This is admirable work, even
> if it does result in some duplication at the LSM layer (and below).
>
> It is my opinion that the impact to the LSM, both at the LSM layer,
> and in the individual affected LSMs is not significant enough to
> outweigh the other advantages offered by this patchset.

Hey, in the end it's your call. I agree that cleaning up kludgy code
is inherently good. I'm willing to believe that putting further effort
into the patch set to make the LSM aspects cleaner isn't cost effective.
If everyone else thinks this is the right approach, I don't need to
question it further.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 18/30] evm: simplify evm_xattr_acl_change()
  2022-09-27 22:56   ` Paul Moore
@ 2022-09-28 13:31     ` Christian Brauner
  0 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2022-09-28 13:31 UTC (permalink / raw)
  To: Paul Moore
  Cc: linux-fsdevel, Seth Forshee, Christoph Hellwig, Al Viro,
	Mimi Zohar, linux-integrity, linux-security-module

On Tue, Sep 27, 2022 at 06:56:44PM -0400, Paul Moore wrote:
> On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > The posix acl api provides a dedicated security and integrity hook for
> > setting posix acls. This means that
> >
> > evm_protect_xattr()
> > -> evm_xattr_change()
> >    -> evm_xattr_acl_change()
> >
> > is now only hit during vfs_remove_acl() at which point we are guaranteed
> > that xattr_value and xattr_value_len are NULL and 0. In this case evm
> > always used to return 1. Simplify this function to do just that.
> >
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> >
> > Notes:
> >     /* v2 */
> >     unchanged
> >
> >  security/integrity/evm/evm_main.c | 62 +++++++------------------------
> >  1 file changed, 14 insertions(+), 48 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 15aa5995fff4..1fbe1b8d0364 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
> >
> >  /*
> >   * evm_xattr_acl_change - check if passed ACL changes the inode mode
> > - * @mnt_userns: user namespace of the idmapped mount
> > - * @dentry: pointer to the affected dentry
> >   * @xattr_name: requested xattr
> >   * @xattr_value: requested xattr value
> >   * @xattr_value_len: requested xattr value length
> >   *
> > - * Check if passed ACL changes the inode mode, which is protected by EVM.
> > + * This is only hit during xattr removal at which point we always return 1.
> > + * Splat a warning in case someone managed to pass data to this function. That
> > + * should never happen.
> >   *
> >   * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> >   */
> > -static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
> > -                               struct dentry *dentry, const char *xattr_name,
> > -                               const void *xattr_value, size_t xattr_value_len)
> > +static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
> >  {
> > -#ifdef CONFIG_FS_POSIX_ACL
> > -       umode_t mode;
> > -       struct posix_acl *acl = NULL, *acl_res;
> > -       struct inode *inode = d_backing_inode(dentry);
> > -       int rc;
> > -
> > -       /*
> > -        * An earlier comment here mentioned that the idmappings for
> > -        * ACL_{GROUP,USER} don't matter since EVM is only interested in the
> > -        * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
> > -        * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
> > -        * representation it should do so correctly. There's no guarantee that
> > -        * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
> > -        * for the mode at some point and it's difficult to keep track of all
> > -        * the LSM and integrity modules and what they do to POSIX ACLs.
> > -        *
> > -        * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
> > -        * ACLs it received. It requires knowledge that only the VFS is
> > -        * guaranteed to have.
> > -        */
> > -       acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
> > -                                 xattr_value, xattr_value_len);
> > -       if (IS_ERR_OR_NULL(acl))
> > -               return 1;
> > -
> > -       acl_res = acl;
> > -       /*
> > -        * Passing mnt_userns is necessary to correctly determine the GID in
> > -        * an idmapped mount, as the GID is used to clear the setgid bit in
> > -        * the inode mode.
> > -        */
> > -       rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
> > -
> > -       posix_acl_release(acl);
> > -
> > -       if (rc)
> > -               return 1;
> > +       int rc = 0;
> >
> > -       if (inode->i_mode != mode)
> > -               return 1;
> > +#ifdef CONFIG_FS_POSIX_ACL
> > +       WARN_ONCE(xattr_value != NULL,
> > +                 "Passing xattr value for POSIX ACLs not supported\n");
> > +       WARN_ONCE(xattr_value_len != 0,
> > +                 "Passing non-zero length for POSIX ACLs not supported\n");
> > +       rc = 1;
> >  #endif
> > -       return 0;
> > +
> > +       return rc;
> >  }
> 
> This is another case where I'll leave the final say up to Mimi, but
> why not just get rid of evm_xattr_acl_change() entirely?  Unless I'm
> missing something, it's only reason for existing now is to check that
> it is passed the proper (empty) parameters which seems pointless ...
> no?

Yeah, I think we can remove it. evm_inode_remove_acl() is just
evm_inode_set_acl(NULL, 0) so if we add evm_inode_remove_acl() as a
wrapper around it instead of simply abusing the existing
evm_inode_removexattr() we can delete all that code indeed as it won't
be reachable from generic xattr code anymore.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-09-28 13:31 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
2022-09-27 22:55   ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 13/30] evm: " Christian Brauner
2022-09-27 22:56   ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 15/30] evm: add post " Christian Brauner
2022-09-27 22:56   ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 18/30] evm: simplify evm_xattr_acl_change() Christian Brauner
2022-09-27 22:56   ` Paul Moore
2022-09-28 13:31     ` Christian Brauner
2022-09-27  0:22 ` [PATCH v2 00/30] acl: add vfs posix acl api Casey Schaufler
2022-09-27  7:41   ` Christoph Hellwig
2022-09-27  7:59     ` Christian Brauner
2022-09-27 14:11     ` Casey Schaufler
2022-09-27 15:16       ` Seth Forshee
2022-09-27 15:55         ` Casey Schaufler
2022-09-27 23:24       ` Paul Moore
2022-09-27 23:37         ` Casey Schaufler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).