[PATCH -next v2 0/6] landlock: add chmod and chown support

[PATCH -next v2 0/6] landlock: add chmod and chown support

[Xiu Jianfeng]

v2:
 * abstract walk_to_visible_parent() helper
 * chmod and chown rights only take affect on directory's context
 * add testcase for fchmodat/lchown/fchownat
 * fix other review issues

Xiu Jianfeng (6):
  landlock: expand access_mask_t to u32 type
  landlock: abstract walk_to_visible_parent() helper
  landlock: add chmod and chown support
  landlock/selftests: add selftests for chmod and chown
  landlock/samples: add chmod and chown support
  landlock: update chmod and chown support in document

 Documentation/userspace-api/landlock.rst     |   9 +-
 include/uapi/linux/landlock.h                |  10 +-
 samples/landlock/sandboxer.c                 |  13 +-
 security/landlock/fs.c                       | 110 ++++++--
 security/landlock/limits.h                   |   2 +-
 security/landlock/ruleset.h                  |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
 9 files changed, 386 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH -next v2 1/6] landlock: expand access_mask_t to u32 type

[Xiu Jianfeng]

u16 is not enough to add more types of restritions, so expand it to u32

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 security/landlock/ruleset.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index d43231b783e4..607b3dc0ef19 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -19,7 +19,7 @@
 #include "limits.h"
 #include "object.h"
 
-typedef u16 access_mask_t;
+typedef u32 access_mask_t;
 /* Makes sure all filesystem access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
 /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
-- 
2.17.1

[PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent() helper

[Xiu Jianfeng]

This helper will be used in the next commit which supports chmod and
chown access rights restriction.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c57f581a9cd5..4ef614a4ea22 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -38,6 +38,44 @@
 #include "ruleset.h"
 #include "setup.h"
 
+enum walk_result {
+	WALK_CONTINUE,
+	WALK_TO_REAL_ROOT,
+	WALK_TO_DISCONN_ROOT,
+};
+
+/*
+ * walk to the visible parent, caller should call path_get()/path_put()
+ * before/after this helpler.
+ *
+ * Returns:
+ * - WALK_TO_REAL_ROOT if walk to the real root;
+ * - WALK_TO_DISCONN_ROOT if walk to disconnected root;
+ * - WALK_CONTINUE otherwise.
+ */
+static enum walk_result walk_to_visible_parent(struct path *path)
+{
+	struct dentry *parent_dentry;
+jump_up:
+	if (path->dentry == path->mnt->mnt_root) {
+		if (follow_up(path)) {
+			/* Ignores hidden mount points. */
+			goto jump_up;
+		} else {
+			/* Stop at the real root. */
+			return WALK_TO_REAL_ROOT;
+		}
+	}
+	/* Stops at disconnected root directories. */
+	if (unlikely(IS_ROOT(path->dentry)))
+		return WALK_TO_DISCONN_ROOT;
+	parent_dentry = dget_parent(path->dentry);
+	dput(path->dentry);
+	path->dentry = parent_dentry;
+
+	return WALK_CONTINUE;
+}
+
 /* Underlying object management */
 
 static void release_inode(struct landlock_object *const object)
@@ -539,8 +577,8 @@ static int check_access_path_dual(
 	 * restriction.
 	 */
 	while (true) {
-		struct dentry *parent_dentry;
 		const struct landlock_rule *rule;
+		enum walk_result wr;
 
 		/*
 		 * If at least all accesses allowed on the destination are
@@ -588,20 +626,12 @@ static int check_access_path_dual(
 		if (allowed_parent1 && allowed_parent2)
 			break;
 
-jump_up:
-		if (walker_path.dentry == walker_path.mnt->mnt_root) {
-			if (follow_up(&walker_path)) {
-				/* Ignores hidden mount points. */
-				goto jump_up;
-			} else {
-				/*
-				 * Stops at the real root.  Denies access
-				 * because not all layers have granted access.
-				 */
-				break;
-			}
-		}
-		if (unlikely(IS_ROOT(walker_path.dentry))) {
+		wr = walk_to_visible_parent(&walker_path);
+		switch (wr) {
+		case WALK_TO_REAL_ROOT:
+			/* Stop at the real root. */
+			goto out;
+		case WALK_TO_DISCONN_ROOT:
 			/*
 			 * Stops at disconnected root directories.  Only allows
 			 * access to internal filesystems (e.g. nsfs, which is
@@ -609,12 +639,13 @@ static int check_access_path_dual(
 			 */
 			allowed_parent1 = allowed_parent2 =
 				!!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
+			goto out;
+		case WALK_CONTINUE:
+		default:
 			break;
 		}
-		parent_dentry = dget_parent(walker_path.dentry);
-		dput(walker_path.dentry);
-		walker_path.dentry = parent_dentry;
 	}
+out:
 	path_put(&walker_path);
 
 	if (allowed_parent1 && allowed_parent2)
-- 
2.17.1

Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent() helper

[Mickaël Salaün]

On 27/08/2022 13:12, Xiu Jianfeng wrote:
> This helper will be used in the next commit which supports chmod and
> chown access rights restriction.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>   security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index c57f581a9cd5..4ef614a4ea22 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -38,6 +38,44 @@
>   #include "ruleset.h"
>   #include "setup.h"
>   
> +enum walk_result {
> +	WALK_CONTINUE,
> +	WALK_TO_REAL_ROOT,
> +	WALK_TO_DISCONN_ROOT,

Why did you created these results instead of the ones I proposed?


> +};
> +
> +/*
> + * walk to the visible parent, caller should call path_get()/path_put()
> + * before/after this helpler.
> + *
> + * Returns:
> + * - WALK_TO_REAL_ROOT if walk to the real root;
> + * - WALK_TO_DISCONN_ROOT if walk to disconnected root;
> + * - WALK_CONTINUE otherwise.
> + */
> +static enum walk_result walk_to_visible_parent(struct path *path)
> +{
> +	struct dentry *parent_dentry;
> +jump_up:
> +	if (path->dentry == path->mnt->mnt_root) {
> +		if (follow_up(path)) {
> +			/* Ignores hidden mount points. */
> +			goto jump_up;
> +		} else {
> +			/* Stop at the real root. */
> +			return WALK_TO_REAL_ROOT;
> +		}
> +	}
> +	/* Stops at disconnected root directories. */
> +	if (unlikely(IS_ROOT(path->dentry)))
> +		return WALK_TO_DISCONN_ROOT;
> +	parent_dentry = dget_parent(path->dentry);
> +	dput(path->dentry);
> +	path->dentry = parent_dentry;
> +
> +	return WALK_CONTINUE;
> +}
> +
>   /* Underlying object management */
>   
>   static void release_inode(struct landlock_object *const object)
> @@ -539,8 +577,8 @@ static int check_access_path_dual(
>   	 * restriction.
>   	 */
>   	while (true) {
> -		struct dentry *parent_dentry;
>   		const struct landlock_rule *rule;
> +		enum walk_result wr;

Please make the names understandable. In this case this variable may not 
be needed anyway.


>   
>   		/*
>   		 * If at least all accesses allowed on the destination are
> @@ -588,20 +626,12 @@ static int check_access_path_dual(
>   		if (allowed_parent1 && allowed_parent2)
>   			break;
>   
> -jump_up:
> -		if (walker_path.dentry == walker_path.mnt->mnt_root) {
> -			if (follow_up(&walker_path)) {
> -				/* Ignores hidden mount points. */
> -				goto jump_up;
> -			} else {
> -				/*
> -				 * Stops at the real root.  Denies access
> -				 * because not all layers have granted access.
> -				 */
> -				break;
> -			}
> -		}
> -		if (unlikely(IS_ROOT(walker_path.dentry))) {
> +		wr = walk_to_visible_parent(&walker_path);
> +		switch (wr) {
> +		case WALK_TO_REAL_ROOT:
> +			/* Stop at the real root. */
> +			goto out;
> +		case WALK_TO_DISCONN_ROOT:
>   			/*
>   			 * Stops at disconnected root directories.  Only allows
>   			 * access to internal filesystems (e.g. nsfs, which is
> @@ -609,12 +639,13 @@ static int check_access_path_dual(
>   			 */
>   			allowed_parent1 = allowed_parent2 =
>   				!!(walker_path.mnt->mnt_flags & MNT_INTERNAL);

Why not include this check in the helper? This is then not checked in 
patch 3 with current_check_access_path_context_only(), which is a bug.


> +			goto out;
> +		case WALK_CONTINUE:
> +		default:
>   			break;
>   		}
> -		parent_dentry = dget_parent(walker_path.dentry);
> -		dput(walker_path.dentry);
> -		walker_path.dentry = parent_dentry;
>   	}
> +out:
>   	path_put(&walker_path);
>   
>   	if (allowed_parent1 && allowed_parent2)

Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent() helper

[xiujianfeng]

Hi,

在 2022/8/30 19:22, Mickaël Salaün 写道:
> 
> On 27/08/2022 13:12, Xiu Jianfeng wrote:
>> This helper will be used in the next commit which supports chmod and
>> chown access rights restriction.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------
>>   1 file changed, 49 insertions(+), 18 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index c57f581a9cd5..4ef614a4ea22 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -38,6 +38,44 @@
>>   #include "ruleset.h"
>>   #include "setup.h"
>> +enum walk_result {
>> +    WALK_CONTINUE,
>> +    WALK_TO_REAL_ROOT,
>> +    WALK_TO_DISCONN_ROOT,
> 
> Why did you created these results instead of the ones I proposed?
> 
> 
>> +};
>> +
>> +/*
>> + * walk to the visible parent, caller should call path_get()/path_put()
>> + * before/after this helpler.
>> + *
>> + * Returns:
>> + * - WALK_TO_REAL_ROOT if walk to the real root;
>> + * - WALK_TO_DISCONN_ROOT if walk to disconnected root;
>> + * - WALK_CONTINUE otherwise.
>> + */
>> +static enum walk_result walk_to_visible_parent(struct path *path)
>> +{
>> +    struct dentry *parent_dentry;
>> +jump_up:
>> +    if (path->dentry == path->mnt->mnt_root) {
>> +        if (follow_up(path)) {
>> +            /* Ignores hidden mount points. */
>> +            goto jump_up;
>> +        } else {
>> +            /* Stop at the real root. */
>> +            return WALK_TO_REAL_ROOT;
>> +        }
>> +    }
>> +    /* Stops at disconnected root directories. */
>> +    if (unlikely(IS_ROOT(path->dentry)))
>> +        return WALK_TO_DISCONN_ROOT;
>> +    parent_dentry = dget_parent(path->dentry);
>> +    dput(path->dentry);
>> +    path->dentry = parent_dentry;
>> +
>> +    return WALK_CONTINUE;
>> +}
>> +
>>   /* Underlying object management */
>>   static void release_inode(struct landlock_object *const object)
>> @@ -539,8 +577,8 @@ static int check_access_path_dual(
>>        * restriction.
>>        */
>>       while (true) {
>> -        struct dentry *parent_dentry;
>>           const struct landlock_rule *rule;
>> +        enum walk_result wr;
> 
> Please make the names understandable. In this case this variable may not 
> be needed anyway.
> 
> 
>>           /*
>>            * If at least all accesses allowed on the destination are
>> @@ -588,20 +626,12 @@ static int check_access_path_dual(
>>           if (allowed_parent1 && allowed_parent2)
>>               break;
>> -jump_up:
>> -        if (walker_path.dentry == walker_path.mnt->mnt_root) {
>> -            if (follow_up(&walker_path)) {
>> -                /* Ignores hidden mount points. */
>> -                goto jump_up;
>> -            } else {
>> -                /*
>> -                 * Stops at the real root.  Denies access
>> -                 * because not all layers have granted access.
>> -                 */
>> -                break;
>> -            }
>> -        }
>> -        if (unlikely(IS_ROOT(walker_path.dentry))) {
>> +        wr = walk_to_visible_parent(&walker_path);
>> +        switch (wr) {
>> +        case WALK_TO_REAL_ROOT:
>> +            /* Stop at the real root. */
>> +            goto out;
>> +        case WALK_TO_DISCONN_ROOT:
>>               /*
>>                * Stops at disconnected root directories.  Only allows
>>                * access to internal filesystems (e.g. nsfs, which is
>> @@ -609,12 +639,13 @@ static int check_access_path_dual(
>>                */
>>               allowed_parent1 = allowed_parent2 =
>>                   !!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
> 
> Why not include this check in the helper? This is then not checked in 
> patch 3 with current_check_access_path_context_only(), which is a bug.

I get your point, after moving it to the helper, here should be:

               while (true) {
                    ...
                    switch(walk_to_visible_parent(&walker_path)) {
                    case WALK_CONTINUE:
                       break;
                    case WALK_ALLOWED:
                       allowed_parent1 = allowed_parent2 = true;
                       goto out;
                    case WR_DENIED:
                    default:
                       allowed_parent1 = allowed_parent2 = false;
                       goto out;
                     }
             }
> 
> 
>> +            goto out;
>> +        case WALK_CONTINUE:
>> +        default:
>>               break;
>>           }
>> -        parent_dentry = dget_parent(walker_path.dentry);
>> -        dput(walker_path.dentry);
>> -        walker_path.dentry = parent_dentry;
>>       }

>> +out:
>>       path_put(&walker_path);
>>       if (allowed_parent1 && allowed_parent2)
> .

[PATCH -next v2 3/6] landlock: add chmod and chown support

[Xiu Jianfeng]

Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHGRP to
support restriction to chmod(2) and chown(2) with landlock.

If these two access rights are set on a directory, they only take effect
for its context, not the directory itself.

This patch also change the landlock ABI version from 3 to 4.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 include/uapi/linux/landlock.h                | 10 +++--
 security/landlock/fs.c                       | 43 +++++++++++++++++++-
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  6 ++-
 6 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 735b1fe8326e..07b73626ff20 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -141,14 +141,16 @@ struct landlock_path_beneath_attr {
  *   directory) parent.  Otherwise, such actions are denied with errno set to
  *   EACCES.  The EACCES errno prevails over EXDEV to let user space
  *   efficiently deal with an unrecoverable error.
+ * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
+ * - %LANDLOCK_ACCESS_FS_CHGRP: Change the owner and/or group of a file.
  *
  * .. warning::
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
- *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
- *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`setxattr(2)`,
+ *   :manpage:`utime(2)`,:manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
+ *   :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
+#define LANDLOCK_ACCESS_FS_CHMOD			(1ULL << 15)
+#define LANDLOCK_ACCESS_FS_CHGRP			(1ULL << 16)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 4ef614a4ea22..6ac83d96ada7 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -185,7 +185,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_CHMOD | \
+	LANDLOCK_ACCESS_FS_CHGRP)
 /* clang-format on */
 
 /*
@@ -690,6 +692,31 @@ static inline int current_check_access_path(const struct path *const path,
 	return check_access_path(dom, path, access_request);
 }
 
+static inline int
+current_check_access_path_context_only(const struct path *const path,
+				       const access_mask_t access_request)
+{
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+	struct path eff_path;
+	int ret;
+
+	if (!dom)
+		return 0;
+	eff_path = *path;
+	/* if it's dir, check its visible parent. */
+	if (d_is_dir(eff_path.dentry)) {
+		path_get(&eff_path);
+		/* dont care if reaches the root or not. */
+		walk_to_visible_parent(&eff_path);
+		ret = current_check_access_path(&eff_path, access_request);
+		path_put(&eff_path);
+	} else {
+		ret = current_check_access_path(&eff_path, access_request);
+	}
+	return ret;
+}
+
 static inline access_mask_t get_mode_access(const umode_t mode)
 {
 	switch (mode & S_IFMT) {
@@ -1177,6 +1204,18 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 
+static int hook_path_chmod(const struct path *const path, umode_t mode)
+{
+	return current_check_access_path_context_only(path,
+					LANDLOCK_ACCESS_FS_CHMOD);
+}
+
+static int hook_path_chown(const struct path *const path, kuid_t uid, kgid_t gid)
+{
+	return current_check_access_path_context_only(path,
+					LANDLOCK_ACCESS_FS_CHGRP);
+}
+
 /* File hooks */
 
 static inline access_mask_t get_file_access(const struct file *const file)
@@ -1230,6 +1269,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
 	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
 	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
+	LSM_HOOK_INIT(path_chmod, hook_path_chmod),
+	LSM_HOOK_INIT(path_chown, hook_path_chown),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 82288f0e9e5e..7cdd7d467d12 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_CHGRP
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index f4d6fc7ed17f..469e0e11735c 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 3
+#define LANDLOCK_ABI_VERSION 4
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 72cdae277b02..9f00582f639c 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index debe2d9ea6cf..f513cd8d9d51 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_CHMOD | \
+	LANDLOCK_ACCESS_FS_CHGRP)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHGRP
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
-- 
2.17.1

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[Günther Noack]

Hello!

the mapping between Landlock rights to LSM hooks is now as follows in
your patch set:

* LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
* LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
  (this hook can restrict both the chown(2) and chgrp(2) syscalls)

Is this the desired mapping?

The previous discussion I found on the topic was in

[1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
[2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
[3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/

In my understanding the main arguments were the ones in [2] and [3].

There were no further responses to [3], so I was under the impression
that we were gravitating towards an approach where the
file-metadata-modification operations were grouped more coarsely?

For example with the approach suggested in [3], which would be to
group the operations coarsely into (a) one Landlock right for
modifying file metadata that is used in security contexts, and (b) one
Landlock right for modifying metadata that was used in non-security
contexts. That would mean that there would be:

(a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
following operations:
  * chmod(2)-variants through hook_path_chmod,
  * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
  * setxattr(2)-variants and removexattr(2)-variants for extended
    attributes that are not "user extended attributes" as described in
    xattr(7) through hook_inode_setxattr and hook_inode_removexattr

(b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
following operations:
  * utimes(2) and other operations for setting other non-security
    sensitive attributes, probably through hook_inode_setattr(?)
  * xattr modifications like above, but for the "user extended
    attributes", though hook_inode_setxattr and hook_inode_removexattr

In my mind, this would be a sensible grouping, and it would also help
to decouple the userspace-exposed API from the underlying
implementation, as Casey suggested to do in [2].

Specifically for this patch set, if you want to use this grouping, you
would only need to add one new Landlock right
(LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
under (a) (and maybe we can find a shorter name for it... :))?

Did I miss any operations here that would be necessary to restrict?

Would that make sense to you? Xiu, what is your opinion on how this
should be grouped? Do you have use cases in mind where a more
fine-grained grouping would be required?

—Günther

P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
on a variety on attribute changes including file ownership, file size
and file mode, so it might potentially interact with a bunch of other
existing Landlock rights. Maybe that is not the right approach. In any
case, it seems like it might require more thinking and it might be
sensible to do that in a separate patch set IMHO.

On Sat, Aug 27, 2022 at 07:12:12PM +0800, Xiu Jianfeng wrote:
> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHGRP to
> support restriction to chmod(2) and chown(2) with landlock.
>
> If these two access rights are set on a directory, they only take effect
> for its context, not the directory itself.
>
> This patch also change the landlock ABI version from 3 to 4.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  include/uapi/linux/landlock.h                | 10 +++--
>  security/landlock/fs.c                       | 43 +++++++++++++++++++-
>  security/landlock/limits.h                   |  2 +-
>  security/landlock/syscalls.c                 |  2 +-
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  tools/testing/selftests/landlock/fs_test.c   |  6 ++-
>  6 files changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 735b1fe8326e..07b73626ff20 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -141,14 +141,16 @@ struct landlock_path_beneath_attr {
>   *   directory) parent.  Otherwise, such actions are denied with errno set to
>   *   EACCES.  The EACCES errno prevails over EXDEV to let user space
>   *   efficiently deal with an unrecoverable error.
> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
> + * - %LANDLOCK_ACCESS_FS_CHGRP: Change the owner and/or group of a file.
>   *
>   * .. warning::
>   *
>   *   It is currently not possible to restrict some file-related actions
>   *   accessible through these syscall families: :manpage:`chdir(2)`,
> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`setxattr(2)`,
> + *   :manpage:`utime(2)`,:manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
> + *   :manpage:`access(2)`.
>   *   Future Landlock evolutions will enable to restrict them.
>   */
>  /* clang-format off */
> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>  #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>  #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>  #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
> +#define LANDLOCK_ACCESS_FS_CHMOD			(1ULL << 15)
> +#define LANDLOCK_ACCESS_FS_CHGRP			(1ULL << 16)
>  /* clang-format on */
>
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 4ef614a4ea22..6ac83d96ada7 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -185,7 +185,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_EXECUTE | \
>  	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>  	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_CHMOD | \
> +	LANDLOCK_ACCESS_FS_CHGRP)
>  /* clang-format on */
>
>  /*
> @@ -690,6 +692,31 @@ static inline int current_check_access_path(const struct path *const path,
>  	return check_access_path(dom, path, access_request);
>  }
>
> +static inline int
> +current_check_access_path_context_only(const struct path *const path,
> +				       const access_mask_t access_request)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +	struct path eff_path;
> +	int ret;
> +
> +	if (!dom)
> +		return 0;
> +	eff_path = *path;
> +	/* if it's dir, check its visible parent. */
> +	if (d_is_dir(eff_path.dentry)) {
> +		path_get(&eff_path);
> +		/* dont care if reaches the root or not. */
> +		walk_to_visible_parent(&eff_path);
> +		ret = current_check_access_path(&eff_path, access_request);
> +		path_put(&eff_path);
> +	} else {
> +		ret = current_check_access_path(&eff_path, access_request);
> +	}
> +	return ret;
> +}
> +
>  static inline access_mask_t get_mode_access(const umode_t mode)
>  {
>  	switch (mode & S_IFMT) {
> @@ -1177,6 +1204,18 @@ static int hook_path_truncate(const struct path *const path)
>  	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>  }
>
> +static int hook_path_chmod(const struct path *const path, umode_t mode)
> +{
> +	return current_check_access_path_context_only(path,
> +					LANDLOCK_ACCESS_FS_CHMOD);
> +}
> +
> +static int hook_path_chown(const struct path *const path, kuid_t uid, kgid_t gid)
> +{
> +	return current_check_access_path_context_only(path,
> +					LANDLOCK_ACCESS_FS_CHGRP);
> +}
> +
>  /* File hooks */
>
>  static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1230,6 +1269,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>  	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>  	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
> +	LSM_HOOK_INIT(path_chmod, hook_path_chmod),
> +	LSM_HOOK_INIT(path_chown, hook_path_chown),
>
>  	LSM_HOOK_INIT(file_open, hook_file_open),
>  };
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 82288f0e9e5e..7cdd7d467d12 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>  #define LANDLOCK_MAX_NUM_LAYERS		16
>  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_CHGRP
>  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index f4d6fc7ed17f..469e0e11735c 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>  	.write = fop_dummy_write,
>  };
>
> -#define LANDLOCK_ABI_VERSION 3
> +#define LANDLOCK_ABI_VERSION 4
>
>  /**
>   * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 72cdae277b02..9f00582f639c 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>  	const struct landlock_ruleset_attr ruleset_attr = {
>  		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>  	};
> -	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
>  					     LANDLOCK_CREATE_RULESET_VERSION));
>
>  	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index debe2d9ea6cf..f513cd8d9d51 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
>  	LANDLOCK_ACCESS_FS_EXECUTE | \
>  	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>  	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_CHMOD | \
> +	LANDLOCK_ACCESS_FS_CHGRP)
>
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHGRP
>
>  #define ACCESS_ALL ( \
>  	ACCESS_FILE | \
> --
> 2.17.1
>

--

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/8/28 3:30, Günther Noack 写道:
> Hello!
> 
> the mapping between Landlock rights to LSM hooks is now as follows in
> your patch set:
> 
> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>    (this hook can restrict both the chown(2) and chgrp(2) syscalls)
> 
> Is this the desired mapping?
> 
> The previous discussion I found on the topic was in
> 
> [1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
> [2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
> [3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
> 
> In my understanding the main arguments were the ones in [2] and [3].
> 
> There were no further responses to [3], so I was under the impression
> that we were gravitating towards an approach where the
> file-metadata-modification operations were grouped more coarsely?
> 
> For example with the approach suggested in [3], which would be to
> group the operations coarsely into (a) one Landlock right for
> modifying file metadata that is used in security contexts, and (b) one
> Landlock right for modifying metadata that was used in non-security
> contexts. That would mean that there would be:
> 
> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
> following operations:
>    * chmod(2)-variants through hook_path_chmod,
>    * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
>    * setxattr(2)-variants and removexattr(2)-variants for extended
>      attributes that are not "user extended attributes" as described in
>      xattr(7) through hook_inode_setxattr and hook_inode_removexattr
> 
> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
> following operations:
>    * utimes(2) and other operations for setting other non-security
>      sensitive attributes, probably through hook_inode_setattr(?)
>    * xattr modifications like above, but for the "user extended
>      attributes", though hook_inode_setxattr and hook_inode_removexattr
> 
> In my mind, this would be a sensible grouping, and it would also help
> to decouple the userspace-exposed API from the underlying
> implementation, as Casey suggested to do in [2].
> 
> Specifically for this patch set, if you want to use this grouping, you
> would only need to add one new Landlock right
> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
> under (a) (and maybe we can find a shorter name for it... :))?
> 
> Did I miss any operations here that would be necessary to restrict?
> 
> Would that make sense to you? Xiu, what is your opinion on how this
> should be grouped? Do you have use cases in mind where a more
> fine-grained grouping would be required?

I apologize I may missed that discussion when I prepared v2:(

Yes, agreed, this grouping is more sensible and resonnable. so in this 
patchset only one right will be added, and I suppose the first commit 
which expand access_mask_t to u32 can be droped.

> 
> —Günther
> 
> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
> on a variety on attribute changes including file ownership, file size
> and file mode, so it might potentially interact with a bunch of other
> existing Landlock rights. Maybe that is not the right approach. In any
> case, it seems like it might require more thinking and it might be
> sensible to do that in a separate patch set IMHO.

Thanks for you reminder, that seems it's more complicated to support 
utimes, so I think we'd better not support it in this patchset.

> 
> On Sat, Aug 27, 2022 at 07:12:12PM +0800, Xiu Jianfeng wrote:
>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHGRP to
>> support restriction to chmod(2) and chown(2) with landlock.
>>
>> If these two access rights are set on a directory, they only take effect
>> for its context, not the directory itself.
>>
>> This patch also change the landlock ABI version from 3 to 4.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   include/uapi/linux/landlock.h                | 10 +++--
>>   security/landlock/fs.c                       | 43 +++++++++++++++++++-
>>   security/landlock/limits.h                   |  2 +-
>>   security/landlock/syscalls.c                 |  2 +-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   tools/testing/selftests/landlock/fs_test.c   |  6 ++-
>>   6 files changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 735b1fe8326e..07b73626ff20 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -141,14 +141,16 @@ struct landlock_path_beneath_attr {
>>    *   directory) parent.  Otherwise, such actions are denied with errno set to
>>    *   EACCES.  The EACCES errno prevails over EXDEV to let user space
>>    *   efficiently deal with an unrecoverable error.
>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
>> + * - %LANDLOCK_ACCESS_FS_CHGRP: Change the owner and/or group of a file.
>>    *
>>    * .. warning::
>>    *
>>    *   It is currently not possible to restrict some file-related actions
>>    *   accessible through these syscall families: :manpage:`chdir(2)`,
>> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`setxattr(2)`,
>> + *   :manpage:`utime(2)`,:manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
>> + *   :manpage:`access(2)`.
>>    *   Future Landlock evolutions will enable to restrict them.
>>    */
>>   /* clang-format off */
>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>> +#define LANDLOCK_ACCESS_FS_CHMOD			(1ULL << 15)
>> +#define LANDLOCK_ACCESS_FS_CHGRP			(1ULL << 16)
>>   /* clang-format on */
>>
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 4ef614a4ea22..6ac83d96ada7 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -185,7 +185,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>   	LANDLOCK_ACCESS_FS_READ_FILE | \
>> -	LANDLOCK_ACCESS_FS_TRUNCATE)
>> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
>> +	LANDLOCK_ACCESS_FS_CHMOD | \
>> +	LANDLOCK_ACCESS_FS_CHGRP)
>>   /* clang-format on */
>>
>>   /*
>> @@ -690,6 +692,31 @@ static inline int current_check_access_path(const struct path *const path,
>>   	return check_access_path(dom, path, access_request);
>>   }
>>
>> +static inline int
>> +current_check_access_path_context_only(const struct path *const path,
>> +				       const access_mask_t access_request)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +	struct path eff_path;
>> +	int ret;
>> +
>> +	if (!dom)
>> +		return 0;
>> +	eff_path = *path;
>> +	/* if it's dir, check its visible parent. */
>> +	if (d_is_dir(eff_path.dentry)) {
>> +		path_get(&eff_path);
>> +		/* dont care if reaches the root or not. */
>> +		walk_to_visible_parent(&eff_path);
>> +		ret = current_check_access_path(&eff_path, access_request);
>> +		path_put(&eff_path);
>> +	} else {
>> +		ret = current_check_access_path(&eff_path, access_request);
>> +	}
>> +	return ret;
>> +}
>> +
>>   static inline access_mask_t get_mode_access(const umode_t mode)
>>   {
>>   	switch (mode & S_IFMT) {
>> @@ -1177,6 +1204,18 @@ static int hook_path_truncate(const struct path *const path)
>>   	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>>   }
>>
>> +static int hook_path_chmod(const struct path *const path, umode_t mode)
>> +{
>> +	return current_check_access_path_context_only(path,
>> +					LANDLOCK_ACCESS_FS_CHMOD);
>> +}
>> +
>> +static int hook_path_chown(const struct path *const path, kuid_t uid, kgid_t gid)
>> +{
>> +	return current_check_access_path_context_only(path,
>> +					LANDLOCK_ACCESS_FS_CHGRP);
>> +}
>> +
>>   /* File hooks */
>>
>>   static inline access_mask_t get_file_access(const struct file *const file)
>> @@ -1230,6 +1269,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>>   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>>   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>>   	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
>> +	LSM_HOOK_INIT(path_chmod, hook_path_chmod),
>> +	LSM_HOOK_INIT(path_chown, hook_path_chown),
>>
>>   	LSM_HOOK_INIT(file_open, hook_file_open),
>>   };
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 82288f0e9e5e..7cdd7d467d12 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -18,7 +18,7 @@
>>   #define LANDLOCK_MAX_NUM_LAYERS		16
>>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>>
>> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
>> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_CHGRP
>>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index f4d6fc7ed17f..469e0e11735c 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>>   	.write = fop_dummy_write,
>>   };
>>
>> -#define LANDLOCK_ABI_VERSION 3
>> +#define LANDLOCK_ABI_VERSION 4
>>
>>   /**
>>    * sys_landlock_create_ruleset - Create a new ruleset
>> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
>> index 72cdae277b02..9f00582f639c 100644
>> --- a/tools/testing/selftests/landlock/base_test.c
>> +++ b/tools/testing/selftests/landlock/base_test.c
>> @@ -75,7 +75,7 @@ TEST(abi_version)
>>   	const struct landlock_ruleset_attr ruleset_attr = {
>>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>>   	};
>> -	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
>> +	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
>>   					     LANDLOCK_CREATE_RULESET_VERSION));
>>
>>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index debe2d9ea6cf..f513cd8d9d51 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
>>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>   	LANDLOCK_ACCESS_FS_READ_FILE | \
>> -	LANDLOCK_ACCESS_FS_TRUNCATE)
>> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
>> +	LANDLOCK_ACCESS_FS_CHMOD | \
>> +	LANDLOCK_ACCESS_FS_CHGRP)
>>
>> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
>> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHGRP
>>
>>   #define ACCESS_ALL ( \
>>   	ACCESS_FILE | \
>> --
>> 2.17.1
>>
> 
> --
> .
> 

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[Mickaël Salaün]

On 29/08/2022 03:17, xiujianfeng wrote:
> 
> Hi,
> 
> 在 2022/8/28 3:30, Günther Noack 写道:
>> Hello!
>>
>> the mapping between Landlock rights to LSM hooks is now as follows in
>> your patch set:
>>
>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>     (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>
>> Is this the desired mapping?
>>
>> The previous discussion I found on the topic was in
>>
>> [1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
>> [2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
>> [3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
>>
>> In my understanding the main arguments were the ones in [2] and [3].
>>
>> There were no further responses to [3], so I was under the impression
>> that we were gravitating towards an approach where the
>> file-metadata-modification operations were grouped more coarsely?
>>
>> For example with the approach suggested in [3], which would be to
>> group the operations coarsely into (a) one Landlock right for
>> modifying file metadata that is used in security contexts, and (b) one
>> Landlock right for modifying metadata that was used in non-security
>> contexts. That would mean that there would be:
>>
>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>> following operations:
>>     * chmod(2)-variants through hook_path_chmod,
>>     * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
>>     * setxattr(2)-variants and removexattr(2)-variants for extended
>>       attributes that are not "user extended attributes" as described in
>>       xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>
>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>> following operations:
>>     * utimes(2) and other operations for setting other non-security
>>       sensitive attributes, probably through hook_inode_setattr(?)
>>     * xattr modifications like above, but for the "user extended
>>       attributes", though hook_inode_setxattr and hook_inode_removexattr
>>
>> In my mind, this would be a sensible grouping, and it would also help
>> to decouple the userspace-exposed API from the underlying
>> implementation, as Casey suggested to do in [2].
>>
>> Specifically for this patch set, if you want to use this grouping, you
>> would only need to add one new Landlock right
>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>> under (a) (and maybe we can find a shorter name for it... :))?
>>
>> Did I miss any operations here that would be necessary to restrict?
>>
>> Would that make sense to you? Xiu, what is your opinion on how this
>> should be grouped? Do you have use cases in mind where a more
>> fine-grained grouping would be required?
> 
> I apologize I may missed that discussion when I prepared v2:(
> 
> Yes, agreed, this grouping is more sensible and resonnable. so in this
> patchset only one right will be added, and I suppose the first commit
> which expand access_mask_t to u32 can be droped.
> 
>>
>> —Günther
>>
>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>> on a variety on attribute changes including file ownership, file size
>> and file mode, so it might potentially interact with a bunch of other
>> existing Landlock rights. Maybe that is not the right approach. In any
>> case, it seems like it might require more thinking and it might be
>> sensible to do that in a separate patch set IMHO.
> 
> Thanks for you reminder, that seems it's more complicated to support
> utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly 
group such access rights. Indeed, to avoid inconsistencies and much more 
complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a 
default (e.g. umask), which is also a way to set them. How to 
consistently manage that? What if the application wants to protect its 
files with chmod 0400?

About the naming, I think we can start with:
- LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
- LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;
- LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel 
(could change non-Landlock DAC or MAC, which could be considered as a 
policy bypass; or other various xattr that might be interpreted by 
filesystems), this should be denied most of the time.

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/8/30 0:01, Mickaël Salaün 写道:
> 
> On 29/08/2022 03:17, xiujianfeng wrote:
>>
>> Hi,
>>
>> 在 2022/8/28 3:30, Günther Noack 写道:
>>> Hello!
>>>
>>> the mapping between Landlock rights to LSM hooks is now as follows in
>>> your patch set:
>>>
>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>     (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>>
>>> Is this the desired mapping?
>>>
>>> The previous discussion I found on the topic was in
>>>
>>> [1] 
>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/ 
>>>
>>> [2] 
>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/ 
>>>
>>> [3] 
>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/ 
>>>
>>>
>>> In my understanding the main arguments were the ones in [2] and [3].
>>>
>>> There were no further responses to [3], so I was under the impression
>>> that we were gravitating towards an approach where the
>>> file-metadata-modification operations were grouped more coarsely?
>>>
>>> For example with the approach suggested in [3], which would be to
>>> group the operations coarsely into (a) one Landlock right for
>>> modifying file metadata that is used in security contexts, and (b) one
>>> Landlock right for modifying metadata that was used in non-security
>>> contexts. That would mean that there would be:
>>>
>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>> following operations:
>>>     * chmod(2)-variants through hook_path_chmod,
>>>     * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
>>>     * setxattr(2)-variants and removexattr(2)-variants for extended
>>>       attributes that are not "user extended attributes" as described in
>>>       xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>>
>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>> following operations:
>>>     * utimes(2) and other operations for setting other non-security
>>>       sensitive attributes, probably through hook_inode_setattr(?)
>>>     * xattr modifications like above, but for the "user extended
>>>       attributes", though hook_inode_setxattr and hook_inode_removexattr
>>>
>>> In my mind, this would be a sensible grouping, and it would also help
>>> to decouple the userspace-exposed API from the underlying
>>> implementation, as Casey suggested to do in [2].
>>>
>>> Specifically for this patch set, if you want to use this grouping, you
>>> would only need to add one new Landlock right
>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>
>>> Did I miss any operations here that would be necessary to restrict?
>>>
>>> Would that make sense to you? Xiu, what is your opinion on how this
>>> should be grouped? Do you have use cases in mind where a more
>>> fine-grained grouping would be required?
>>
>> I apologize I may missed that discussion when I prepared v2:(
>>
>> Yes, agreed, this grouping is more sensible and resonnable. so in this
>> patchset only one right will be added, and I suppose the first commit
>> which expand access_mask_t to u32 can be droped.
>>
>>>
>>> —Günther
>>>
>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>>> on a variety on attribute changes including file ownership, file size
>>> and file mode, so it might potentially interact with a bunch of other
>>> existing Landlock rights. Maybe that is not the right approach. In any
>>> case, it seems like it might require more thinking and it might be
>>> sensible to do that in a separate patch set IMHO.
>>
>> Thanks for you reminder, that seems it's more complicated to support
>> utimes, so I think we'd better not support it in this patchset.
> 
> The issue with this approach is that it makes it impossible to properly 
> group such access rights. Indeed, to avoid inconsistencies and much more 
> complexity, we cannot extend a Landlock access right once it is defined.
> 
> We also need to consider that file ownership and permissions have a 
> default (e.g. umask), which is also a way to set them. How to 
> consistently manage that? What if the application wants to protect its 
> files with chmod 0400?

what do you mean by this? do you mean that we should have a set of 
default permissions for files created by applications within the 
sandbox, so that it can update metadata of its own file.

> 
> About the naming, I think we can start with:
> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;

do you mean we should have permission controls on metadata level or 
operation level? e.g. should we allow update on user xattr but deny 
update on security xattr? or should we disallow update on any xattr?

> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel 
> (could change non-Landlock DAC or MAC, which could be considered as a 
> policy bypass; or other various xattr that might be interpreted by 
> filesystems), this should be denied most of the time.

do you mean FS_WRITE_UNSAFE_METADATA is security-related? and 
FS_WRITE_SAFE_METADATA is non-security-related?

> .

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[Mickaël Salaün]

CCing linux-fsdevel@vger.kernel.org


On 01/09/2022 15:06, xiujianfeng wrote:
> Hi,
> 
> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>
>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>
>>> Hi,
>>>
>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>> Hello!
>>>>
>>>> the mapping between Landlock rights to LSM hooks is now as follows in
>>>> your patch set:
>>>>
>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>      (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>>>
>>>> Is this the desired mapping?
>>>>
>>>> The previous discussion I found on the topic was in
>>>>
>>>> [1]
>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
>>>>
>>>> [3]
>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
>>>>
>>>>
>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>
>>>> There were no further responses to [3], so I was under the impression
>>>> that we were gravitating towards an approach where the
>>>> file-metadata-modification operations were grouped more coarsely?
>>>>
>>>> For example with the approach suggested in [3], which would be to
>>>> group the operations coarsely into (a) one Landlock right for
>>>> modifying file metadata that is used in security contexts, and (b) one
>>>> Landlock right for modifying metadata that was used in non-security
>>>> contexts. That would mean that there would be:
>>>>
>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>> following operations:
>>>>      * chmod(2)-variants through hook_path_chmod,
>>>>      * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
>>>>      * setxattr(2)-variants and removexattr(2)-variants for extended
>>>>        attributes that are not "user extended attributes" as described in
>>>>        xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>>>
>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>> following operations:
>>>>      * utimes(2) and other operations for setting other non-security
>>>>        sensitive attributes, probably through hook_inode_setattr(?)
>>>>      * xattr modifications like above, but for the "user extended
>>>>        attributes", though hook_inode_setxattr and hook_inode_removexattr
>>>>
>>>> In my mind, this would be a sensible grouping, and it would also help
>>>> to decouple the userspace-exposed API from the underlying
>>>> implementation, as Casey suggested to do in [2].
>>>>
>>>> Specifically for this patch set, if you want to use this grouping, you
>>>> would only need to add one new Landlock right
>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>
>>>> Did I miss any operations here that would be necessary to restrict?
>>>>
>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>> should be grouped? Do you have use cases in mind where a more
>>>> fine-grained grouping would be required?
>>>
>>> I apologize I may missed that discussion when I prepared v2:(
>>>
>>> Yes, agreed, this grouping is more sensible and resonnable. so in this
>>> patchset only one right will be added, and I suppose the first commit
>>> which expand access_mask_t to u32 can be droped.
>>>
>>>>
>>>> —Günther
>>>>
>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>>>> on a variety on attribute changes including file ownership, file size
>>>> and file mode, so it might potentially interact with a bunch of other
>>>> existing Landlock rights. Maybe that is not the right approach. In any
>>>> case, it seems like it might require more thinking and it might be
>>>> sensible to do that in a separate patch set IMHO.
>>>
>>> Thanks for you reminder, that seems it's more complicated to support
>>> utimes, so I think we'd better not support it in this patchset.
>>
>> The issue with this approach is that it makes it impossible to properly
>> group such access rights. Indeed, to avoid inconsistencies and much more
>> complexity, we cannot extend a Landlock access right once it is defined.
>>
>> We also need to consider that file ownership and permissions have a
>> default (e.g. umask), which is also a way to set them. How to
>> consistently manage that? What if the application wants to protect its
>> files with chmod 0400?
> 
> what do you mean by this? do you mean that we should have a set of
> default permissions for files created by applications within the
> sandbox, so that it can update metadata of its own file.

I mean that we need a consistent access control system, and for this we 
need to consider all the ways an extended attribute can be set.

We can either extend the meaning of current access rights (controlled 
with a ruleset flag for compatibility reasons), or create new access 
rights. I think it would be better to add new dedicated rights to make 
it more explicit and flexible.

I'm not sure about the right approach to properly control file 
permission. We need to think about it. Do you have some ideas?

BTW, utimes can be controlled with the inode_setattr() LSM hook. Being 
able to control arbitrary file time modification could be part of the 
FS_WRITE_SAFE_METADATA, but modification and access time should always 
be updated according to the file operation.


> 
>>
>> About the naming, I think we can start with:
>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;
> 
> do you mean we should have permission controls on metadata level or
> operation level? e.g. should we allow update on user xattr but deny
> update on security xattr? or should we disallow update on any xattr?
> 
>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>> (could change non-Landlock DAC or MAC, which could be considered as a
>> policy bypass; or other various xattr that might be interpreted by
>> filesystems), this should be denied most of the time.
> 
> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
> FS_WRITE_SAFE_METADATA is non-security-related?

Yes, FS_WRITE_UNSAFE_METADATA would be for security related 
xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. 
Both are mutually exclusive. This would involve the inode_setattr and 
inode_setxattr LSM hooks. Looking at the calling sites, it seems 
possible to replace all inode arguments with paths.

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/9/2 1:34, Mickaël Salaün 写道:
> CCing linux-fsdevel@vger.kernel.org
> 
> 
> On 01/09/2022 15:06, xiujianfeng wrote:
>> Hi,
>>
>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>
>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>
>>>> Hi,
>>>>
>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>> Hello!
>>>>>
>>>>> the mapping between Landlock rights to LSM hooks is now as follows in
>>>>> your patch set:
>>>>>
>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>      (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>>>>
>>>>> Is this the desired mapping?
>>>>>
>>>>> The previous discussion I found on the topic was in
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/ 
>>>>>
>>>>>
>>>>> [2]
>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/ 
>>>>>
>>>>>
>>>>> [3]
>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/ 
>>>>>
>>>>>
>>>>>
>>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>>
>>>>> There were no further responses to [3], so I was under the impression
>>>>> that we were gravitating towards an approach where the
>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>
>>>>> For example with the approach suggested in [3], which would be to
>>>>> group the operations coarsely into (a) one Landlock right for
>>>>> modifying file metadata that is used in security contexts, and (b) one
>>>>> Landlock right for modifying metadata that was used in non-security
>>>>> contexts. That would mean that there would be:
>>>>>
>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>> following operations:
>>>>>      * chmod(2)-variants through hook_path_chmod,
>>>>>      * chown(2)-variants and chgrp(2)-variants through 
>>>>> hook_path_chown,
>>>>>      * setxattr(2)-variants and removexattr(2)-variants for extended
>>>>>        attributes that are not "user extended attributes" as 
>>>>> described in
>>>>>        xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>>>>
>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>>> following operations:
>>>>>      * utimes(2) and other operations for setting other non-security
>>>>>        sensitive attributes, probably through hook_inode_setattr(?)
>>>>>      * xattr modifications like above, but for the "user extended
>>>>>        attributes", though hook_inode_setxattr and 
>>>>> hook_inode_removexattr
>>>>>
>>>>> In my mind, this would be a sensible grouping, and it would also help
>>>>> to decouple the userspace-exposed API from the underlying
>>>>> implementation, as Casey suggested to do in [2].
>>>>>
>>>>> Specifically for this patch set, if you want to use this grouping, you
>>>>> would only need to add one new Landlock right
>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>
>>>>> Did I miss any operations here that would be necessary to restrict?
>>>>>
>>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>>> should be grouped? Do you have use cases in mind where a more
>>>>> fine-grained grouping would be required?
>>>>
>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>
>>>> Yes, agreed, this grouping is more sensible and resonnable. so in this
>>>> patchset only one right will be added, and I suppose the first commit
>>>> which expand access_mask_t to u32 can be droped.
>>>>
>>>>>
>>>>> —Günther
>>>>>
>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>>>>> on a variety on attribute changes including file ownership, file size
>>>>> and file mode, so it might potentially interact with a bunch of other
>>>>> existing Landlock rights. Maybe that is not the right approach. In any
>>>>> case, it seems like it might require more thinking and it might be
>>>>> sensible to do that in a separate patch set IMHO.
>>>>
>>>> Thanks for you reminder, that seems it's more complicated to support
>>>> utimes, so I think we'd better not support it in this patchset.
>>>
>>> The issue with this approach is that it makes it impossible to properly
>>> group such access rights. Indeed, to avoid inconsistencies and much more
>>> complexity, we cannot extend a Landlock access right once it is defined.
>>>
>>> We also need to consider that file ownership and permissions have a
>>> default (e.g. umask), which is also a way to set them. How to
>>> consistently manage that? What if the application wants to protect its
>>> files with chmod 0400?
>>
>> what do you mean by this? do you mean that we should have a set of
>> default permissions for files created by applications within the
>> sandbox, so that it can update metadata of its own file.
> 
> I mean that we need a consistent access control system, and for this we 
> need to consider all the ways an extended attribute can be set.
> 
> We can either extend the meaning of current access rights (controlled 
> with a ruleset flag for compatibility reasons), or create new access 
> rights. I think it would be better to add new dedicated rights to make 
> it more explicit and flexible.
> 
> I'm not sure about the right approach to properly control file 
> permission. We need to think about it. Do you have some ideas?
> 
> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being 
> able to control arbitrary file time modification could be part of the 
> FS_WRITE_SAFE_METADATA, but modification and access time should always 
> be updated according to the file operation.
> 
> 
>>
>>>
>>> About the naming, I think we can start with:
>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;
>>
>> do you mean we should have permission controls on metadata level or
>> operation level? e.g. should we allow update on user xattr but deny
>> update on security xattr? or should we disallow update on any xattr?
>>
>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>>> (could change non-Landlock DAC or MAC, which could be considered as a
>>> policy bypass; or other various xattr that might be interpreted by
>>> filesystems), this should be denied most of the time.
>>
>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>> FS_WRITE_SAFE_METADATA is non-security-related?
> 
> Yes, FS_WRITE_UNSAFE_METADATA would be for security related 
> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. 
> Both are mutually exclusive. This would involve the inode_setattr and 
> inode_setxattr LSM hooks. Looking at the calling sites, it seems 
> possible to replace all inode arguments with paths.
> .

Sorry for the late reply, I have problems with this work, for example,
before:
security_inode_setattr(struct user_namespace *mnt_userns,
                                          struct dentry *dentry,
                                          struct iattr *attr)
after:
security_inode_setattr(struct user_namespace *mnt_userns,
                                          struct path *path,
                                          struct iattr *attr)
then I change the second argument in notify_change() from struct *dentry 
to struct path *, that makes this kind of changes in fs/overlayfs/ 
spread to lots of places because overlayfs basicly uses dentry instead 
of path, the worst case may be here:

ovl_special_inode_operations.set_acl hook calls:
-->
ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, 
struct posix_acl *acl, int type)
-->
ovl_setattr(struct user_namespace *mnt_userns, struct dentry 
*dentry,struct iattr *attr)
-->
ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry, 
struct iattr *attr)

from the top of this callchain, I can not find a path to replace dentry, 
did I miss something? or do you have better idea?

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[Mickaël Salaün]

On 29/10/2022 10:33, xiujianfeng wrote:
> Hi,
> 
> 在 2022/9/2 1:34, Mickaël Salaün 写道:
>> CCing linux-fsdevel@vger.kernel.org
>>
>>
>> On 01/09/2022 15:06, xiujianfeng wrote:
>>> Hi,
>>>
>>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>>
>>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>>> Hello!
>>>>>>
>>>>>> the mapping between Landlock rights to LSM hooks is now as follows in
>>>>>> your patch set:
>>>>>>
>>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>>       (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>>>>>
>>>>>> Is this the desired mapping?
>>>>>>
>>>>>> The previous discussion I found on the topic was in
>>>>>>
>>>>>> [1]
>>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
>>>>>>
>>>>>>
>>>>>> [2]
>>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
>>>>>>
>>>>>>
>>>>>> [3]
>>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
>>>>>>
>>>>>>
>>>>>>
>>>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>>>
>>>>>> There were no further responses to [3], so I was under the impression
>>>>>> that we were gravitating towards an approach where the
>>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>>
>>>>>> For example with the approach suggested in [3], which would be to
>>>>>> group the operations coarsely into (a) one Landlock right for
>>>>>> modifying file metadata that is used in security contexts, and (b) one
>>>>>> Landlock right for modifying metadata that was used in non-security
>>>>>> contexts. That would mean that there would be:
>>>>>>
>>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>>> following operations:
>>>>>>       * chmod(2)-variants through hook_path_chmod,
>>>>>>       * chown(2)-variants and chgrp(2)-variants through
>>>>>> hook_path_chown,
>>>>>>       * setxattr(2)-variants and removexattr(2)-variants for extended
>>>>>>         attributes that are not "user extended attributes" as
>>>>>> described in
>>>>>>         xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>>>>>
>>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>>>> following operations:
>>>>>>       * utimes(2) and other operations for setting other non-security
>>>>>>         sensitive attributes, probably through hook_inode_setattr(?)
>>>>>>       * xattr modifications like above, but for the "user extended
>>>>>>         attributes", though hook_inode_setxattr and
>>>>>> hook_inode_removexattr
>>>>>>
>>>>>> In my mind, this would be a sensible grouping, and it would also help
>>>>>> to decouple the userspace-exposed API from the underlying
>>>>>> implementation, as Casey suggested to do in [2].
>>>>>>
>>>>>> Specifically for this patch set, if you want to use this grouping, you
>>>>>> would only need to add one new Landlock right
>>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>>
>>>>>> Did I miss any operations here that would be necessary to restrict?
>>>>>>
>>>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>>>> should be grouped? Do you have use cases in mind where a more
>>>>>> fine-grained grouping would be required?
>>>>>
>>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>>
>>>>> Yes, agreed, this grouping is more sensible and resonnable. so in this
>>>>> patchset only one right will be added, and I suppose the first commit
>>>>> which expand access_mask_t to u32 can be droped.
>>>>>
>>>>>>
>>>>>> —Günther
>>>>>>
>>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>>>>>> on a variety on attribute changes including file ownership, file size
>>>>>> and file mode, so it might potentially interact with a bunch of other
>>>>>> existing Landlock rights. Maybe that is not the right approach. In any
>>>>>> case, it seems like it might require more thinking and it might be
>>>>>> sensible to do that in a separate patch set IMHO.
>>>>>
>>>>> Thanks for you reminder, that seems it's more complicated to support
>>>>> utimes, so I think we'd better not support it in this patchset.
>>>>
>>>> The issue with this approach is that it makes it impossible to properly
>>>> group such access rights. Indeed, to avoid inconsistencies and much more
>>>> complexity, we cannot extend a Landlock access right once it is defined.
>>>>
>>>> We also need to consider that file ownership and permissions have a
>>>> default (e.g. umask), which is also a way to set them. How to
>>>> consistently manage that? What if the application wants to protect its
>>>> files with chmod 0400?
>>>
>>> what do you mean by this? do you mean that we should have a set of
>>> default permissions for files created by applications within the
>>> sandbox, so that it can update metadata of its own file.
>>
>> I mean that we need a consistent access control system, and for this we
>> need to consider all the ways an extended attribute can be set.
>>
>> We can either extend the meaning of current access rights (controlled
>> with a ruleset flag for compatibility reasons), or create new access
>> rights. I think it would be better to add new dedicated rights to make
>> it more explicit and flexible.
>>
>> I'm not sure about the right approach to properly control file
>> permission. We need to think about it. Do you have some ideas?
>>
>> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being
>> able to control arbitrary file time modification could be part of the
>> FS_WRITE_SAFE_METADATA, but modification and access time should always
>> be updated according to the file operation.
>>
>>
>>>
>>>>
>>>> About the naming, I think we can start with:
>>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;
>>>
>>> do you mean we should have permission controls on metadata level or
>>> operation level? e.g. should we allow update on user xattr but deny
>>> update on security xattr? or should we disallow update on any xattr?
>>>
>>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>>>> (could change non-Landlock DAC or MAC, which could be considered as a
>>>> policy bypass; or other various xattr that might be interpreted by
>>>> filesystems), this should be denied most of the time.
>>>
>>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>>> FS_WRITE_SAFE_METADATA is non-security-related?
>>
>> Yes, FS_WRITE_UNSAFE_METADATA would be for security related
>> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr.
>> Both are mutually exclusive. This would involve the inode_setattr and
>> inode_setxattr LSM hooks. Looking at the calling sites, it seems
>> possible to replace all inode arguments with paths.

I though about differentiating user xattr, atime/mtime, DAC 
(chown/chmod, posix ACLs), and other xattr, but it would be too complex 
to get a consistent approach because of indirect consequences (e.g. 
controlling umask, setegid, settimeofday…). Let's make it simple for now.

Here is an update on my previous proposal:

LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e. 
inode attr and xattr). In practice, for most use cases, this access 
right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.

LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr 
or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted 
that file modification time and access time should always be updated 
according to the file operation (e.g. write, truncate) even when this 
access is not explicitly allowed (according to vfs_utimes(), 
ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from 
implicit time changes).


> 
> Sorry for the late reply, I have problems with this work, for example,
> before:
> security_inode_setattr(struct user_namespace *mnt_userns,
>                                            struct dentry *dentry,
>                                            struct iattr *attr)
> after:
> security_inode_setattr(struct user_namespace *mnt_userns,
>                                            struct path *path,
>                                            struct iattr *attr)
> then I change the second argument in notify_change() from struct *dentry
> to struct path *, that makes this kind of changes in fs/overlayfs/
> spread to lots of places because overlayfs basicly uses dentry instead
> of path, the worst case may be here:
> 
> ovl_special_inode_operations.set_acl hook calls:
> -->
> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> struct posix_acl *acl, int type)
> -->
> ovl_setattr(struct user_namespace *mnt_userns, struct dentry
> *dentry,struct iattr *attr)
> -->
> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry,
> struct iattr *attr)
> 
> from the top of this callchain, I can not find a path to replace dentry,
> did I miss something? or do you have better idea?

I think this can be solved thanks to the ovl_path_real() helper.

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

在 2022/11/14 22:12, Mickaël Salaün 写道:
> 
> On 29/10/2022 10:33, xiujianfeng wrote:
>> Hi,
>>
>> 在 2022/9/2 1:34, Mickaël Salaün 写道:
>>> CCing linux-fsdevel@vger.kernel.org
>>>
>>>
>>> On 01/09/2022 15:06, xiujianfeng wrote:
>>>> Hi,
>>>>
>>>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>>>
>>>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>>>> Hello!
>>>>>>>
>>>>>>> the mapping between Landlock rights to LSM hooks is now as 
>>>>>>> follows in
>>>>>>> your patch set:
>>>>>>>
>>>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>>>       (this hook can restrict both the chown(2) and chgrp(2) 
>>>>>>> syscalls)
>>>>>>>
>>>>>>> Is this the desired mapping?
>>>>>>>
>>>>>>> The previous discussion I found on the topic was in
>>>>>>>
>>>>>>> [1]
>>>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [2]
>>>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [3]
>>>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>>>>
>>>>>>> There were no further responses to [3], so I was under the 
>>>>>>> impression
>>>>>>> that we were gravitating towards an approach where the
>>>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>>>
>>>>>>> For example with the approach suggested in [3], which would be to
>>>>>>> group the operations coarsely into (a) one Landlock right for
>>>>>>> modifying file metadata that is used in security contexts, and 
>>>>>>> (b) one
>>>>>>> Landlock right for modifying metadata that was used in non-security
>>>>>>> contexts. That would mean that there would be:
>>>>>>>
>>>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>>>> following operations:
>>>>>>>       * chmod(2)-variants through hook_path_chmod,
>>>>>>>       * chown(2)-variants and chgrp(2)-variants through
>>>>>>> hook_path_chown,
>>>>>>>       * setxattr(2)-variants and removexattr(2)-variants for 
>>>>>>> extended
>>>>>>>         attributes that are not "user extended attributes" as
>>>>>>> described in
>>>>>>>         xattr(7) through hook_inode_setxattr and 
>>>>>>> hook_inode_removexattr
>>>>>>>
>>>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>>>>> following operations:
>>>>>>>       * utimes(2) and other operations for setting other 
>>>>>>> non-security
>>>>>>>         sensitive attributes, probably through hook_inode_setattr(?)
>>>>>>>       * xattr modifications like above, but for the "user extended
>>>>>>>         attributes", though hook_inode_setxattr and
>>>>>>> hook_inode_removexattr
>>>>>>>
>>>>>>> In my mind, this would be a sensible grouping, and it would also 
>>>>>>> help
>>>>>>> to decouple the userspace-exposed API from the underlying
>>>>>>> implementation, as Casey suggested to do in [2].
>>>>>>>
>>>>>>> Specifically for this patch set, if you want to use this 
>>>>>>> grouping, you
>>>>>>> would only need to add one new Landlock right
>>>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>>>
>>>>>>> Did I miss any operations here that would be necessary to restrict?
>>>>>>>
>>>>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>>>>> should be grouped? Do you have use cases in mind where a more
>>>>>>> fine-grained grouping would be required?
>>>>>>
>>>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>>>
>>>>>> Yes, agreed, this grouping is more sensible and resonnable. so in 
>>>>>> this
>>>>>> patchset only one right will be added, and I suppose the first commit
>>>>>> which expand access_mask_t to u32 can be droped.
>>>>>>
>>>>>>>
>>>>>>> —Günther
>>>>>>>
>>>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets 
>>>>>>> called
>>>>>>> on a variety on attribute changes including file ownership, file 
>>>>>>> size
>>>>>>> and file mode, so it might potentially interact with a bunch of 
>>>>>>> other
>>>>>>> existing Landlock rights. Maybe that is not the right approach. 
>>>>>>> In any
>>>>>>> case, it seems like it might require more thinking and it might be
>>>>>>> sensible to do that in a separate patch set IMHO.
>>>>>>
>>>>>> Thanks for you reminder, that seems it's more complicated to support
>>>>>> utimes, so I think we'd better not support it in this patchset.
>>>>>
>>>>> The issue with this approach is that it makes it impossible to 
>>>>> properly
>>>>> group such access rights. Indeed, to avoid inconsistencies and much 
>>>>> more
>>>>> complexity, we cannot extend a Landlock access right once it is 
>>>>> defined.
>>>>>
>>>>> We also need to consider that file ownership and permissions have a
>>>>> default (e.g. umask), which is also a way to set them. How to
>>>>> consistently manage that? What if the application wants to protect its
>>>>> files with chmod 0400?
>>>>
>>>> what do you mean by this? do you mean that we should have a set of
>>>> default permissions for files created by applications within the
>>>> sandbox, so that it can update metadata of its own file.
>>>
>>> I mean that we need a consistent access control system, and for this we
>>> need to consider all the ways an extended attribute can be set.
>>>
>>> We can either extend the meaning of current access rights (controlled
>>> with a ruleset flag for compatibility reasons), or create new access
>>> rights. I think it would be better to add new dedicated rights to make
>>> it more explicit and flexible.
>>>
>>> I'm not sure about the right approach to properly control file
>>> permission. We need to think about it. Do you have some ideas?
>>>
>>> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being
>>> able to control arbitrary file time modification could be part of the
>>> FS_WRITE_SAFE_METADATA, but modification and access time should always
>>> be updated according to the file operation.
>>>
>>>
>>>>
>>>>>
>>>>> About the naming, I think we can start with:
>>>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user 
>>>>> xattr;
>>>>
>>>> do you mean we should have permission controls on metadata level or
>>>> operation level? e.g. should we allow update on user xattr but deny
>>>> update on security xattr? or should we disallow update on any xattr?
>>>>
>>>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>>>>> (could change non-Landlock DAC or MAC, which could be considered as a
>>>>> policy bypass; or other various xattr that might be interpreted by
>>>>> filesystems), this should be denied most of the time.
>>>>
>>>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>>>> FS_WRITE_SAFE_METADATA is non-security-related?
>>>
>>> Yes, FS_WRITE_UNSAFE_METADATA would be for security related
>>> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr.
>>> Both are mutually exclusive. This would involve the inode_setattr and
>>> inode_setxattr LSM hooks. Looking at the calling sites, it seems
>>> possible to replace all inode arguments with paths.
> 
> I though about differentiating user xattr, atime/mtime, DAC 
> (chown/chmod, posix ACLs), and other xattr, but it would be too complex 
> to get a consistent approach because of indirect consequences (e.g. 
> controlling umask, setegid, settimeofday…). Let's make it simple for now.
> 
> Here is an update on my previous proposal:
> 
> LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e. 
> inode attr and xattr). In practice, for most use cases, this access 
> right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.
> 
> LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr 
> or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted 
> that file modification time and access time should always be updated 
> according to the file operation (e.g. write, truncate) even when this 
> access is not explicitly allowed (according to vfs_utimes(), 
> ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from 
> implicit time changes).
> 
Thanks, I analyzed the relevant functions and the use of lsm hooks.
so I think what to do will be as follows:

LANDLOCK_ACCESS_FS_WRITE_METADATA controls the following hooks:
1.security_path_chmod
2.security_path_chown
3.security_inode_setattr
4.security_inode_setxattr
5.security_inode_removexattr
6.security_inode_set_acl

LANDLOCK_ACCESS_FS_READ_METADATA controls the following hooks:
1.security_inode_getattr
2.security_inode_get_acl
3.security_inode_getxattr

and the following 7 hooks are using struct dentry * as parameter, should 
be changed to struct path *, and also their callers.

security_inode_setattr
security_inode_setxattr
security_inode_removexattr
security_inode_set_acl
security_inode_getattr
security_inode_get_acl
security_inode_getxattr

Looks like it's a big change.

> 
>>
>> Sorry for the late reply, I have problems with this work, for example,
>> before:
>> security_inode_setattr(struct user_namespace *mnt_userns,
>>                                            struct dentry *dentry,
>>                                            struct iattr *attr)
>> after:
>> security_inode_setattr(struct user_namespace *mnt_userns,
>>                                            struct path *path,
>>                                            struct iattr *attr)
>> then I change the second argument in notify_change() from struct *dentry
>> to struct path *, that makes this kind of changes in fs/overlayfs/
>> spread to lots of places because overlayfs basicly uses dentry instead
>> of path, the worst case may be here:
>>
>> ovl_special_inode_operations.set_acl hook calls:
>> -->
>> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
>> struct posix_acl *acl, int type)
>> -->
>> ovl_setattr(struct user_namespace *mnt_userns, struct dentry
>> *dentry,struct iattr *attr)
>> -->
>> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry,
>> struct iattr *attr)
>>
>> from the top of this callchain, I can not find a path to replace dentry,
>> did I miss something? or do you have better idea?
> 
> I think this can be solved thanks to the ovl_path_real() helper.
> .

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[Mickaël Salaün]

On 18/11/2022 10:03, xiujianfeng wrote:
> 
> 
> 在 2022/11/14 22:12, Mickaël Salaün 写道:
>>
>> On 29/10/2022 10:33, xiujianfeng wrote:
>>> Hi,
>>>
>>> 在 2022/9/2 1:34, Mickaël Salaün 写道:
>>>> CCing linux-fsdevel@vger.kernel.org
>>>>
>>>>
>>>> On 01/09/2022 15:06, xiujianfeng wrote:
>>>>> Hi,
>>>>>
>>>>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>>>>
>>>>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> the mapping between Landlock rights to LSM hooks is now as
>>>>>>>> follows in
>>>>>>>> your patch set:
>>>>>>>>
>>>>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>>>>        (this hook can restrict both the chown(2) and chgrp(2)
>>>>>>>> syscalls)
>>>>>>>>
>>>>>>>> Is this the desired mapping?
>>>>>>>>
>>>>>>>> The previous discussion I found on the topic was in
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [2]
>>>>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [3]
>>>>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>>>>>
>>>>>>>> There were no further responses to [3], so I was under the
>>>>>>>> impression
>>>>>>>> that we were gravitating towards an approach where the
>>>>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>>>>
>>>>>>>> For example with the approach suggested in [3], which would be to
>>>>>>>> group the operations coarsely into (a) one Landlock right for
>>>>>>>> modifying file metadata that is used in security contexts, and
>>>>>>>> (b) one
>>>>>>>> Landlock right for modifying metadata that was used in non-security
>>>>>>>> contexts. That would mean that there would be:
>>>>>>>>
>>>>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>>>>> following operations:
>>>>>>>>        * chmod(2)-variants through hook_path_chmod,
>>>>>>>>        * chown(2)-variants and chgrp(2)-variants through
>>>>>>>> hook_path_chown,
>>>>>>>>        * setxattr(2)-variants and removexattr(2)-variants for
>>>>>>>> extended
>>>>>>>>          attributes that are not "user extended attributes" as
>>>>>>>> described in
>>>>>>>>          xattr(7) through hook_inode_setxattr and
>>>>>>>> hook_inode_removexattr
>>>>>>>>
>>>>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>>>>>> following operations:
>>>>>>>>        * utimes(2) and other operations for setting other
>>>>>>>> non-security
>>>>>>>>          sensitive attributes, probably through hook_inode_setattr(?)
>>>>>>>>        * xattr modifications like above, but for the "user extended
>>>>>>>>          attributes", though hook_inode_setxattr and
>>>>>>>> hook_inode_removexattr
>>>>>>>>
>>>>>>>> In my mind, this would be a sensible grouping, and it would also
>>>>>>>> help
>>>>>>>> to decouple the userspace-exposed API from the underlying
>>>>>>>> implementation, as Casey suggested to do in [2].
>>>>>>>>
>>>>>>>> Specifically for this patch set, if you want to use this
>>>>>>>> grouping, you
>>>>>>>> would only need to add one new Landlock right
>>>>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>>>>
>>>>>>>> Did I miss any operations here that would be necessary to restrict?
>>>>>>>>
>>>>>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>>>>>> should be grouped? Do you have use cases in mind where a more
>>>>>>>> fine-grained grouping would be required?
>>>>>>>
>>>>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>>>>
>>>>>>> Yes, agreed, this grouping is more sensible and resonnable. so in
>>>>>>> this
>>>>>>> patchset only one right will be added, and I suppose the first commit
>>>>>>> which expand access_mask_t to u32 can be droped.
>>>>>>>
>>>>>>>>
>>>>>>>> —Günther
>>>>>>>>
>>>>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets
>>>>>>>> called
>>>>>>>> on a variety on attribute changes including file ownership, file
>>>>>>>> size
>>>>>>>> and file mode, so it might potentially interact with a bunch of
>>>>>>>> other
>>>>>>>> existing Landlock rights. Maybe that is not the right approach.
>>>>>>>> In any
>>>>>>>> case, it seems like it might require more thinking and it might be
>>>>>>>> sensible to do that in a separate patch set IMHO.
>>>>>>>
>>>>>>> Thanks for you reminder, that seems it's more complicated to support
>>>>>>> utimes, so I think we'd better not support it in this patchset.
>>>>>>
>>>>>> The issue with this approach is that it makes it impossible to
>>>>>> properly
>>>>>> group such access rights. Indeed, to avoid inconsistencies and much
>>>>>> more
>>>>>> complexity, we cannot extend a Landlock access right once it is
>>>>>> defined.
>>>>>>
>>>>>> We also need to consider that file ownership and permissions have a
>>>>>> default (e.g. umask), which is also a way to set them. How to
>>>>>> consistently manage that? What if the application wants to protect its
>>>>>> files with chmod 0400?
>>>>>
>>>>> what do you mean by this? do you mean that we should have a set of
>>>>> default permissions for files created by applications within the
>>>>> sandbox, so that it can update metadata of its own file.
>>>>
>>>> I mean that we need a consistent access control system, and for this we
>>>> need to consider all the ways an extended attribute can be set.
>>>>
>>>> We can either extend the meaning of current access rights (controlled
>>>> with a ruleset flag for compatibility reasons), or create new access
>>>> rights. I think it would be better to add new dedicated rights to make
>>>> it more explicit and flexible.
>>>>
>>>> I'm not sure about the right approach to properly control file
>>>> permission. We need to think about it. Do you have some ideas?
>>>>
>>>> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being
>>>> able to control arbitrary file time modification could be part of the
>>>> FS_WRITE_SAFE_METADATA, but modification and access time should always
>>>> be updated according to the file operation.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> About the naming, I think we can start with:
>>>>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>>>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user
>>>>>> xattr;
>>>>>
>>>>> do you mean we should have permission controls on metadata level or
>>>>> operation level? e.g. should we allow update on user xattr but deny
>>>>> update on security xattr? or should we disallow update on any xattr?
>>>>>
>>>>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>>>>>> (could change non-Landlock DAC or MAC, which could be considered as a
>>>>>> policy bypass; or other various xattr that might be interpreted by
>>>>>> filesystems), this should be denied most of the time.
>>>>>
>>>>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>>>>> FS_WRITE_SAFE_METADATA is non-security-related?
>>>>
>>>> Yes, FS_WRITE_UNSAFE_METADATA would be for security related
>>>> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr.
>>>> Both are mutually exclusive. This would involve the inode_setattr and
>>>> inode_setxattr LSM hooks. Looking at the calling sites, it seems
>>>> possible to replace all inode arguments with paths.
>>
>> I though about differentiating user xattr, atime/mtime, DAC
>> (chown/chmod, posix ACLs), and other xattr, but it would be too complex
>> to get a consistent approach because of indirect consequences (e.g.
>> controlling umask, setegid, settimeofday…). Let's make it simple for now.
>>
>> Here is an update on my previous proposal:
>>
>> LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e.
>> inode attr and xattr). In practice, for most use cases, this access
>> right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.
>>
>> LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr
>> or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted
>> that file modification time and access time should always be updated
>> according to the file operation (e.g. write, truncate) even when this
>> access is not explicitly allowed (according to vfs_utimes(),
>> ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from
>> implicit time changes).
>>
> Thanks, I analyzed the relevant functions and the use of lsm hooks.
> so I think what to do will be as follows:
> 
> LANDLOCK_ACCESS_FS_WRITE_METADATA controls the following hooks:
> 1.security_path_chmod
> 2.security_path_chown

These two chmod/chown hooks would be redundant with 
security_inode_setattr(). We then don't need to implement them.


> 3.security_inode_setattr
> 4.security_inode_setxattr
> 5.security_inode_removexattr > 6.security_inode_set_acl

Good catch. This new security_inode_set_acl hook is a good example of 
API refactoring. BTW, the related Cc list should be included in your 
next patch series.

> 
> LANDLOCK_ACCESS_FS_READ_METADATA controls the following hooks:
> 1.security_inode_getattr
> 2.security_inode_get_acl
> 3.security_inode_getxattr

Correct

> 
> and the following 7 hooks are using struct dentry * as parameter, should
> be changed to struct path *, and also their callers.
> 
> security_inode_setattr
> security_inode_setxattr
> security_inode_removexattr
> security_inode_set_acl
> security_inode_getattr
> security_inode_get_acl
> security_inode_getxattr
> 
> Looks like it's a big change.

Your proposed approach looks good, and this will indeed touch a lot of 
files.

Because it interacts a lot with the filesystem subsystem, I propose to 
first write a set of patches that refactor the security_inode_*attr and 
security_inode_*_acl hooks to use struct file (or struct path when it 
makes sense) instead of struct dentry/inode (and to remove struct 
user_namespace as argument because it can be inferred thanks to 
file_mnt_user_ns). As for [1], using struct file only makes sense for a 
specific set of calls, and struct path should be used otherwise (e.g. 
syscalls dealing with file descriptors vs. with file paths).

You need to base this work on Christian's branch to be up-to-date with 
ongoing FS changes. I suggest to create one patch per function API 
change e.g., notify_change (merge the mnt_userns and dentry in a file 
argument), struct inode_operations.setattr (use a file argument instead 
of dentry)…

Once this refactoring will be in -next, the landlock_file_security 
changes [1] will already be merged in master, and you will then be able 
to work on the Landlock specific parts with the new hooks.

[1] https://git.kernel.org/mic/c/b9f5ce27c8f8


> 
>>
>>>
>>> Sorry for the late reply, I have problems with this work, for example,
>>> before:
>>> security_inode_setattr(struct user_namespace *mnt_userns,
>>>                                             struct dentry *dentry,
>>>                                             struct iattr *attr)
>>> after:
>>> security_inode_setattr(struct user_namespace *mnt_userns,
>>>                                             struct path *path,
>>>                                             struct iattr *attr)
>>> then I change the second argument in notify_change() from struct *dentry
>>> to struct path *, that makes this kind of changes in fs/overlayfs/
>>> spread to lots of places because overlayfs basicly uses dentry instead
>>> of path, the worst case may be here:
>>>
>>> ovl_special_inode_operations.set_acl hook calls:
>>> -->
>>> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
>>> struct posix_acl *acl, int type)
>>> -->
>>> ovl_setattr(struct user_namespace *mnt_userns, struct dentry
>>> *dentry,struct iattr *attr)
>>> -->
>>> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry,
>>> struct iattr *attr)
>>>
>>> from the top of this callchain, I can not find a path to replace dentry,
>>> did I miss something? or do you have better idea?
>>
>> I think this can be solved thanks to the ovl_path_real() helper.
>> .

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/11/18 20:32, Mickaël Salaün 写道:
> 
> On 18/11/2022 10:03, xiujianfeng wrote:
>>
>>
>> 在 2022/11/14 22:12, Mickaël Salaün 写道:
>>>
>>> On 29/10/2022 10:33, xiujianfeng wrote:
>>>> Hi,
>>>>
>>>> 在 2022/9/2 1:34, Mickaël Salaün 写道:
>>>>> CCing linux-fsdevel@vger.kernel.org
>>>>>
>>>>>
>>>>> On 01/09/2022 15:06, xiujianfeng wrote:
>>>>>> Hi,
>>>>>>
>>>>>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>>>>>
>>>>>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>>>>>> Hello!
>>>>>>>>>
>>>>>>>>> the mapping between Landlock rights to LSM hooks is now as
>>>>>>>>> follows in
>>>>>>>>> your patch set:
>>>>>>>>>
>>>>>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>>>>>        (this hook can restrict both the chown(2) and chgrp(2)
>>>>>>>>> syscalls)
>>>>>>>>>
>>>>>>>>> Is this the desired mapping?
>>>>>>>>>
>>>>>>>>> The previous discussion I found on the topic was in
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [2]
>>>>>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [3]
>>>>>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> In my understanding the main arguments were the ones in [2] and 
>>>>>>>>> [3].
>>>>>>>>>
>>>>>>>>> There were no further responses to [3], so I was under the
>>>>>>>>> impression
>>>>>>>>> that we were gravitating towards an approach where the
>>>>>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>>>>>
>>>>>>>>> For example with the approach suggested in [3], which would be to
>>>>>>>>> group the operations coarsely into (a) one Landlock right for
>>>>>>>>> modifying file metadata that is used in security contexts, and
>>>>>>>>> (b) one
>>>>>>>>> Landlock right for modifying metadata that was used in 
>>>>>>>>> non-security
>>>>>>>>> contexts. That would mean that there would be:
>>>>>>>>>
>>>>>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>>>>>> following operations:
>>>>>>>>>        * chmod(2)-variants through hook_path_chmod,
>>>>>>>>>        * chown(2)-variants and chgrp(2)-variants through
>>>>>>>>> hook_path_chown,
>>>>>>>>>        * setxattr(2)-variants and removexattr(2)-variants for
>>>>>>>>> extended
>>>>>>>>>          attributes that are not "user extended attributes" as
>>>>>>>>> described in
>>>>>>>>>          xattr(7) through hook_inode_setxattr and
>>>>>>>>> hook_inode_removexattr
>>>>>>>>>
>>>>>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to 
>>>>>>>>> control the
>>>>>>>>> following operations:
>>>>>>>>>        * utimes(2) and other operations for setting other
>>>>>>>>> non-security
>>>>>>>>>          sensitive attributes, probably through 
>>>>>>>>> hook_inode_setattr(?)
>>>>>>>>>        * xattr modifications like above, but for the "user 
>>>>>>>>> extended
>>>>>>>>>          attributes", though hook_inode_setxattr and
>>>>>>>>> hook_inode_removexattr
>>>>>>>>>
>>>>>>>>> In my mind, this would be a sensible grouping, and it would also
>>>>>>>>> help
>>>>>>>>> to decouple the userspace-exposed API from the underlying
>>>>>>>>> implementation, as Casey suggested to do in [2].
>>>>>>>>>
>>>>>>>>> Specifically for this patch set, if you want to use this
>>>>>>>>> grouping, you
>>>>>>>>> would only need to add one new Landlock right
>>>>>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>>>>>
>>>>>>>>> Did I miss any operations here that would be necessary to 
>>>>>>>>> restrict?
>>>>>>>>>
>>>>>>>>> Would that make sense to you? Xiu, what is your opinion on how 
>>>>>>>>> this
>>>>>>>>> should be grouped? Do you have use cases in mind where a more
>>>>>>>>> fine-grained grouping would be required?
>>>>>>>>
>>>>>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>>>>>
>>>>>>>> Yes, agreed, this grouping is more sensible and resonnable. so in
>>>>>>>> this
>>>>>>>> patchset only one right will be added, and I suppose the first 
>>>>>>>> commit
>>>>>>>> which expand access_mask_t to u32 can be droped.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> —Günther
>>>>>>>>>
>>>>>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets
>>>>>>>>> called
>>>>>>>>> on a variety on attribute changes including file ownership, file
>>>>>>>>> size
>>>>>>>>> and file mode, so it might potentially interact with a bunch of
>>>>>>>>> other
>>>>>>>>> existing Landlock rights. Maybe that is not the right approach.
>>>>>>>>> In any
>>>>>>>>> case, it seems like it might require more thinking and it might be
>>>>>>>>> sensible to do that in a separate patch set IMHO.
>>>>>>>>
>>>>>>>> Thanks for you reminder, that seems it's more complicated to 
>>>>>>>> support
>>>>>>>> utimes, so I think we'd better not support it in this patchset.
>>>>>>>
>>>>>>> The issue with this approach is that it makes it impossible to
>>>>>>> properly
>>>>>>> group such access rights. Indeed, to avoid inconsistencies and much
>>>>>>> more
>>>>>>> complexity, we cannot extend a Landlock access right once it is
>>>>>>> defined.
>>>>>>>
>>>>>>> We also need to consider that file ownership and permissions have a
>>>>>>> default (e.g. umask), which is also a way to set them. How to
>>>>>>> consistently manage that? What if the application wants to 
>>>>>>> protect its
>>>>>>> files with chmod 0400?
>>>>>>
>>>>>> what do you mean by this? do you mean that we should have a set of
>>>>>> default permissions for files created by applications within the
>>>>>> sandbox, so that it can update metadata of its own file.
>>>>>
>>>>> I mean that we need a consistent access control system, and for 
>>>>> this we
>>>>> need to consider all the ways an extended attribute can be set.
>>>>>
>>>>> We can either extend the meaning of current access rights (controlled
>>>>> with a ruleset flag for compatibility reasons), or create new access
>>>>> rights. I think it would be better to add new dedicated rights to make
>>>>> it more explicit and flexible.
>>>>>
>>>>> I'm not sure about the right approach to properly control file
>>>>> permission. We need to think about it. Do you have some ideas?
>>>>>
>>>>> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being
>>>>> able to control arbitrary file time modification could be part of the
>>>>> FS_WRITE_SAFE_METADATA, but modification and access time should always
>>>>> be updated according to the file operation.
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> About the naming, I think we can start with:
>>>>>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>>>>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user
>>>>>>> xattr;
>>>>>>
>>>>>> do you mean we should have permission controls on metadata level or
>>>>>> operation level? e.g. should we allow update on user xattr but deny
>>>>>> update on security xattr? or should we disallow update on any xattr?
>>>>>>
>>>>>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the 
>>>>>>> kernel
>>>>>>> (could change non-Landlock DAC or MAC, which could be considered 
>>>>>>> as a
>>>>>>> policy bypass; or other various xattr that might be interpreted by
>>>>>>> filesystems), this should be denied most of the time.
>>>>>>
>>>>>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>>>>>> FS_WRITE_SAFE_METADATA is non-security-related?
>>>>>
>>>>> Yes, FS_WRITE_UNSAFE_METADATA would be for security related
>>>>> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr.
>>>>> Both are mutually exclusive. This would involve the inode_setattr and
>>>>> inode_setxattr LSM hooks. Looking at the calling sites, it seems
>>>>> possible to replace all inode arguments with paths.
>>>
>>> I though about differentiating user xattr, atime/mtime, DAC
>>> (chown/chmod, posix ACLs), and other xattr, but it would be too complex
>>> to get a consistent approach because of indirect consequences (e.g.
>>> controlling umask, setegid, settimeofday…). Let's make it simple for 
>>> now.
>>>
>>> Here is an update on my previous proposal:
>>>
>>> LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e.
>>> inode attr and xattr). In practice, for most use cases, this access
>>> right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.
>>>
>>> LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr
>>> or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted
>>> that file modification time and access time should always be updated
>>> according to the file operation (e.g. write, truncate) even when this
>>> access is not explicitly allowed (according to vfs_utimes(),
>>> ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from
>>> implicit time changes).
>>>
>> Thanks, I analyzed the relevant functions and the use of lsm hooks.
>> so I think what to do will be as follows:
>>
>> LANDLOCK_ACCESS_FS_WRITE_METADATA controls the following hooks:
>> 1.security_path_chmod
>> 2.security_path_chown
> 
> These two chmod/chown hooks would be redundant with 
> security_inode_setattr(). We then don't need to implement them.
> 
> 
>> 3.security_inode_setattr
>> 4.security_inode_setxattr
>> 5.security_inode_removexattr > 6.security_inode_set_acl
> 
> Good catch. This new security_inode_set_acl hook is a good example of 
> API refactoring. BTW, the related Cc list should be included in your 
> next patch series.
> 
>>
>> LANDLOCK_ACCESS_FS_READ_METADATA controls the following hooks:
>> 1.security_inode_getattr
>> 2.security_inode_get_acl
>> 3.security_inode_getxattr
> 
> Correct
> 
>>
>> and the following 7 hooks are using struct dentry * as parameter, should
>> be changed to struct path *, and also their callers.
>>
>> security_inode_setattr
>> security_inode_setxattr
>> security_inode_removexattr
>> security_inode_set_acl
>> security_inode_getattr
>> security_inode_get_acl
>> security_inode_getxattr
>>
>> Looks like it's a big change.
> 
> Your proposed approach looks good, and this will indeed touch a lot of 
> files.
> 
> Because it interacts a lot with the filesystem subsystem, I propose to 
> first write a set of patches that refactor the security_inode_*attr and 
> security_inode_*_acl hooks to use struct file (or struct path when it 
> makes sense) instead of struct dentry/inode (and to remove struct 
> user_namespace as argument because it can be inferred thanks to 
> file_mnt_user_ns). As for [1], using struct file only makes sense for a 
> specific set of calls, and struct path should be used otherwise (e.g. 
> syscalls dealing with file descriptors vs. with file paths).
> 
> You need to base this work on Christian's branch to be up-to-date with 
> ongoing FS changes. I suggest to create one patch per function API 
> change e.g., notify_change (merge the mnt_userns and dentry in a file 
> argument), struct inode_operations.setattr (use a file argument instead 
> of dentry)…


Thanks Mickaël, your advice is very clear, I will do it first.


> 
> Once this refactoring will be in -next, the landlock_file_security 
> changes [1] will already be merged in master, and you will then be able 
> to work on the Landlock specific parts with the new hooks.
> 
> [1] https://git.kernel.org/mic/c/b9f5ce27c8f8
> 
> 
>>
>>>
>>>>
>>>> Sorry for the late reply, I have problems with this work, for example,
>>>> before:
>>>> security_inode_setattr(struct user_namespace *mnt_userns,
>>>>                                             struct dentry *dentry,
>>>>                                             struct iattr *attr)
>>>> after:
>>>> security_inode_setattr(struct user_namespace *mnt_userns,
>>>>                                             struct path *path,
>>>>                                             struct iattr *attr)
>>>> then I change the second argument in notify_change() from struct 
>>>> *dentry
>>>> to struct path *, that makes this kind of changes in fs/overlayfs/
>>>> spread to lots of places because overlayfs basicly uses dentry instead
>>>> of path, the worst case may be here:
>>>>
>>>> ovl_special_inode_operations.set_acl hook calls:
>>>> -->
>>>> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
>>>> struct posix_acl *acl, int type)
>>>> -->
>>>> ovl_setattr(struct user_namespace *mnt_userns, struct dentry
>>>> *dentry,struct iattr *attr)
>>>> -->
>>>> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry,
>>>> struct iattr *attr)
>>>>
>>>> from the top of this callchain, I can not find a path to replace 
>>>> dentry,
>>>> did I miss something? or do you have better idea?
>>>
>>> I think this can be solved thanks to the ovl_path_real() helper.
>>> .
> .

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/8/28 3:30, Günther Noack 写道:
> Hello!
> 
> the mapping between Landlock rights to LSM hooks is now as follows in
> your patch set:
> 
> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>    (this hook can restrict both the chown(2) and chgrp(2) syscalls)
> 
> Is this the desired mapping?
> 
> The previous discussion I found on the topic was in
> 
> [1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/
> [2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/
> [3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/
> 
> In my understanding the main arguments were the ones in [2] and [3].
> 
> There were no further responses to [3], so I was under the impression
> that we were gravitating towards an approach where the
> file-metadata-modification operations were grouped more coarsely?
> 
> For example with the approach suggested in [3], which would be to
> group the operations coarsely into (a) one Landlock right for
> modifying file metadata that is used in security contexts, and (b) one
> Landlock right for modifying metadata that was used in non-security
> contexts. That would mean that there would be:
> 
> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
> following operations:

LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES looks too long, can we use 
LANDLOCK_ACCESS_FS_MOD_SEC_ATTR and LANDLOCK_ACCESS_FS_MOD_NONSEC_ATTR?

>    * chmod(2)-variants through hook_path_chmod,
>    * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
>    * setxattr(2)-variants and removexattr(2)-variants for extended
>      attributes that are not "user extended attributes" as described in
>      xattr(7) through hook_inode_setxattr and hook_inode_removexattr
> 
> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
> following operations:
>    * utimes(2) and other operations for setting other non-security
>      sensitive attributes, probably through hook_inode_setattr(?)
>    * xattr modifications like above, but for the "user extended
>      attributes", though hook_inode_setxattr and hook_inode_removexattr
> 
> In my mind, this would be a sensible grouping, and it would also help
> to decouple the userspace-exposed API from the underlying
> implementation, as Casey suggested to do in [2].
> 
> Specifically for this patch set, if you want to use this grouping, you
> would only need to add one new Landlock right
> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
> under (a) (and maybe we can find a shorter name for it... :))?
> 
> Did I miss any operations here that would be necessary to restrict?
> 
> Would that make sense to you? Xiu, what is your opinion on how this
> should be grouped? Do you have use cases in mind where a more
> fine-grained grouping would be required?
> 
> —Günther
> 
> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
> on a variety on attribute changes including file ownership, file size
> and file mode, so it might potentially interact with a bunch of other
> existing Landlock rights. Maybe that is not the right approach. In any
> case, it seems like it might require more thinking and it might be
> sensible to do that in a separate patch set IMHO.
> 
> On Sat, Aug 27, 2022 at 07:12:12PM +0800, Xiu Jianfeng wrote:
>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHGRP to
>> support restriction to chmod(2) and chown(2) with landlock.
>>
>> If these two access rights are set on a directory, they only take effect
>> for its context, not the directory itself.
>>
>> This patch also change the landlock ABI version from 3 to 4.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   include/uapi/linux/landlock.h                | 10 +++--
>>   security/landlock/fs.c                       | 43 +++++++++++++++++++-
>>   security/landlock/limits.h                   |  2 +-
>>   security/landlock/syscalls.c                 |  2 +-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   tools/testing/selftests/landlock/fs_test.c   |  6 ++-
>>   6 files changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 735b1fe8326e..07b73626ff20 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -141,14 +141,16 @@ struct landlock_path_beneath_attr {
>>    *   directory) parent.  Otherwise, such actions are denied with errno set to
>>    *   EACCES.  The EACCES errno prevails over EXDEV to let user space
>>    *   efficiently deal with an unrecoverable error.
>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
>> + * - %LANDLOCK_ACCESS_FS_CHGRP: Change the owner and/or group of a file.
>>    *
>>    * .. warning::
>>    *
>>    *   It is currently not possible to restrict some file-related actions
>>    *   accessible through these syscall families: :manpage:`chdir(2)`,
>> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`setxattr(2)`,
>> + *   :manpage:`utime(2)`,:manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
>> + *   :manpage:`access(2)`.
>>    *   Future Landlock evolutions will enable to restrict them.
>>    */
>>   /* clang-format off */
>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>> +#define LANDLOCK_ACCESS_FS_CHMOD			(1ULL << 15)
>> +#define LANDLOCK_ACCESS_FS_CHGRP			(1ULL << 16)
>>   /* clang-format on */
>>
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 4ef614a4ea22..6ac83d96ada7 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -185,7 +185,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>   	LANDLOCK_ACCESS_FS_READ_FILE | \
>> -	LANDLOCK_ACCESS_FS_TRUNCATE)
>> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
>> +	LANDLOCK_ACCESS_FS_CHMOD | \
>> +	LANDLOCK_ACCESS_FS_CHGRP)
>>   /* clang-format on */
>>
>>   /*
>> @@ -690,6 +692,31 @@ static inline int current_check_access_path(const struct path *const path,
>>   	return check_access_path(dom, path, access_request);
>>   }
>>
>> +static inline int
>> +current_check_access_path_context_only(const struct path *const path,
>> +				       const access_mask_t access_request)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +	struct path eff_path;
>> +	int ret;
>> +
>> +	if (!dom)
>> +		return 0;
>> +	eff_path = *path;
>> +	/* if it's dir, check its visible parent. */
>> +	if (d_is_dir(eff_path.dentry)) {
>> +		path_get(&eff_path);
>> +		/* dont care if reaches the root or not. */
>> +		walk_to_visible_parent(&eff_path);
>> +		ret = current_check_access_path(&eff_path, access_request);
>> +		path_put(&eff_path);
>> +	} else {
>> +		ret = current_check_access_path(&eff_path, access_request);
>> +	}
>> +	return ret;
>> +}
>> +
>>   static inline access_mask_t get_mode_access(const umode_t mode)
>>   {
>>   	switch (mode & S_IFMT) {
>> @@ -1177,6 +1204,18 @@ static int hook_path_truncate(const struct path *const path)
>>   	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>>   }
>>
>> +static int hook_path_chmod(const struct path *const path, umode_t mode)
>> +{
>> +	return current_check_access_path_context_only(path,
>> +					LANDLOCK_ACCESS_FS_CHMOD);
>> +}
>> +
>> +static int hook_path_chown(const struct path *const path, kuid_t uid, kgid_t gid)
>> +{
>> +	return current_check_access_path_context_only(path,
>> +					LANDLOCK_ACCESS_FS_CHGRP);
>> +}
>> +
>>   /* File hooks */
>>
>>   static inline access_mask_t get_file_access(const struct file *const file)
>> @@ -1230,6 +1269,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>>   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>>   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>>   	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
>> +	LSM_HOOK_INIT(path_chmod, hook_path_chmod),
>> +	LSM_HOOK_INIT(path_chown, hook_path_chown),
>>
>>   	LSM_HOOK_INIT(file_open, hook_file_open),
>>   };
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 82288f0e9e5e..7cdd7d467d12 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -18,7 +18,7 @@
>>   #define LANDLOCK_MAX_NUM_LAYERS		16
>>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>>
>> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
>> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_CHGRP
>>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index f4d6fc7ed17f..469e0e11735c 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>>   	.write = fop_dummy_write,
>>   };
>>
>> -#define LANDLOCK_ABI_VERSION 3
>> +#define LANDLOCK_ABI_VERSION 4
>>
>>   /**
>>    * sys_landlock_create_ruleset - Create a new ruleset
>> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
>> index 72cdae277b02..9f00582f639c 100644
>> --- a/tools/testing/selftests/landlock/base_test.c
>> +++ b/tools/testing/selftests/landlock/base_test.c
>> @@ -75,7 +75,7 @@ TEST(abi_version)
>>   	const struct landlock_ruleset_attr ruleset_attr = {
>>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>>   	};
>> -	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
>> +	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
>>   					     LANDLOCK_CREATE_RULESET_VERSION));
>>
>>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index debe2d9ea6cf..f513cd8d9d51 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
>>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>   	LANDLOCK_ACCESS_FS_READ_FILE | \
>> -	LANDLOCK_ACCESS_FS_TRUNCATE)
>> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
>> +	LANDLOCK_ACCESS_FS_CHMOD | \
>> +	LANDLOCK_ACCESS_FS_CHGRP)
>>
>> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
>> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHGRP
>>
>>   #define ACCESS_ALL ( \
>>   	ACCESS_FILE | \
>> --
>> 2.17.1
>>
> 
> --
> .
> 

Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng]

Hi,

在 2022/8/27 19:12, Xiu Jianfeng 写道:
> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHGRP to
> support restriction to chmod(2) and chown(2) with landlock.
> 
> If these two access rights are set on a directory, they only take effect
> for its context, not the directory itself.
> 
> This patch also change the landlock ABI version from 3 to 4.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>   include/uapi/linux/landlock.h                | 10 +++--
>   security/landlock/fs.c                       | 43 +++++++++++++++++++-
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   |  6 ++-
>   6 files changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 735b1fe8326e..07b73626ff20 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -141,14 +141,16 @@ struct landlock_path_beneath_attr {
>    *   directory) parent.  Otherwise, such actions are denied with errno set to
>    *   EACCES.  The EACCES errno prevails over EXDEV to let user space
>    *   efficiently deal with an unrecoverable error.
> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
> + * - %LANDLOCK_ACCESS_FS_CHGRP: Change the owner and/or group of a file.
>    *
>    * .. warning::
>    *
>    *   It is currently not possible to restrict some file-related actions
>    *   accessible through these syscall families: :manpage:`chdir(2)`,
> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> + *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`setxattr(2)`,
> + *   :manpage:`utime(2)`,:manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
> + *   :manpage:`access(2)`.
>    *   Future Landlock evolutions will enable to restrict them.
>    */
>   /* clang-format off */
> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
> +#define LANDLOCK_ACCESS_FS_CHMOD			(1ULL << 15)
> +#define LANDLOCK_ACCESS_FS_CHGRP			(1ULL << 16)
>   /* clang-format on */
>   
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 4ef614a4ea22..6ac83d96ada7 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -185,7 +185,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>   	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_CHMOD | \
> +	LANDLOCK_ACCESS_FS_CHGRP)
>   /* clang-format on */
>   
>   /*
> @@ -690,6 +692,31 @@ static inline int current_check_access_path(const struct path *const path,
>   	return check_access_path(dom, path, access_request);
>   }
>   
> +static inline int
> +current_check_access_path_context_only(const struct path *const path,
> +				       const access_mask_t access_request)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +	struct path eff_path;
> +	int ret;
> +
> +	if (!dom)
> +		return 0;
> +	eff_path = *path;
> +	/* if it's dir, check its visible parent. */
> +	if (d_is_dir(eff_path.dentry)) {
> +		path_get(&eff_path);
> +		/* dont care if reaches the root or not. */

I may made a mistake here, I think it should return -EACCES directly if 
the walk result is not WALK_CONTINUE.

> +		walk_to_visible_parent(&eff_path);
> +		ret = current_check_access_path(&eff_path, access_request);
> +		path_put(&eff_path);
> +	} else {
> +		ret = current_check_access_path(&eff_path, access_request);
> +	}
> +	return ret;
> +}
> +
>   static inline access_mask_t get_mode_access(const umode_t mode)
>   {
>   	switch (mode & S_IFMT) {
> @@ -1177,6 +1204,18 @@ static int hook_path_truncate(const struct path *const path)
>   	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>   }
>   
> +static int hook_path_chmod(const struct path *const path, umode_t mode)
> +{
> +	return current_check_access_path_context_only(path,
> +					LANDLOCK_ACCESS_FS_CHMOD);
> +}
> +
> +static int hook_path_chown(const struct path *const path, kuid_t uid, kgid_t gid)
> +{
> +	return current_check_access_path_context_only(path,
> +					LANDLOCK_ACCESS_FS_CHGRP);
> +}
> +
>   /* File hooks */
>   
>   static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1230,6 +1269,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>   	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>   	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
> +	LSM_HOOK_INIT(path_chmod, hook_path_chmod),
> +	LSM_HOOK_INIT(path_chown, hook_path_chown),
>   
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   };
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 82288f0e9e5e..7cdd7d467d12 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>   #define LANDLOCK_MAX_NUM_LAYERS		16
>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>   
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_CHGRP
>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>   
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index f4d6fc7ed17f..469e0e11735c 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>   	.write = fop_dummy_write,
>   };
>   
> -#define LANDLOCK_ABI_VERSION 3
> +#define LANDLOCK_ABI_VERSION 4
>   
>   /**
>    * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 72cdae277b02..9f00582f639c 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>   	const struct landlock_ruleset_attr ruleset_attr = {
>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>   	};
> -	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
>   					     LANDLOCK_CREATE_RULESET_VERSION));
>   
>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index debe2d9ea6cf..f513cd8d9d51 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>   	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_CHMOD | \
> +	LANDLOCK_ACCESS_FS_CHGRP)
>   
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHGRP
>   
>   #define ACCESS_ALL ( \
>   	ACCESS_FILE | \
> 

[PATCH -next v2 4/6] landlock/selftests: add selftests for chmod and chown

[Xiu Jianfeng]

The following APIs are tested with simple scenarios.
1. chmod/fchmod/fchmodat;
2. chmod/fchmod/lchown/fchownat;

The key point is that set these access rights on a directory but only for
its content, not the directory itself. this scenario is covered.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 tools/testing/selftests/landlock/fs_test.c | 261 +++++++++++++++++++++
 1 file changed, 261 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index f513cd8d9d51..982cb824967c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3272,6 +3272,267 @@ TEST_F_FORK(layout1, truncate)
 	EXPECT_EQ(0, test_creat(file_in_dir_w));
 }
 
+/* Invokes chmod(2) and returns its errno or 0. */
+static int test_chmod(const char *const path, mode_t mode)
+{
+	if (chmod(path, mode) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes fchmod(2) and returns its errno or 0. */
+static int test_fchmod(int fd, mode_t mode)
+{
+	if (fchmod(fd, mode) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes fchmodat(2) and returns its errno or 0. */
+static int test_fchmodat(int dirfd, const char *path, mode_t mode, int flags)
+{
+	if (fchmodat(dirfd, path, mode, flags) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes chown(2) and returns its errno or 0. */
+static int test_chown(const char *path, uid_t uid, gid_t gid)
+{
+	if (chown(path, uid, gid) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes fchown(2) and returns its errno or 0. */
+static int test_fchown(int fd, uid_t uid, gid_t gid)
+{
+	if (fchown(fd, uid, gid) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes lchown(2) and returns its errno or 0. */
+static int test_lchown(const char *path, uid_t uid, gid_t gid)
+{
+	if (lchown(path, uid, gid) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes fchownat(2) and returns its errno or 0. */
+static int test_fchownat(int dirfd, const char *path,
+			 uid_t uid, gid_t gid, int flags)
+{
+	if (fchownat(dirfd, path, uid, gid, flags) < 0)
+		return errno;
+	return 0;
+}
+
+TEST_F_FORK(layout1, unhandled_chmod)
+{
+	int ruleset_fd, file1_fd;
+	const char *file1 = file1_s1d1;
+	const char *file2 = file2_s1d1;
+	const char *dir1 = dir_s1d1;
+	const struct rule rules[] = {
+		{
+			.path = file1,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = dir1,
+			.access = ACCESS_RW,
+		},
+		{},
+	};
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+	ASSERT_LE(0, ruleset_fd);
+	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file1_fd);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(0, test_chmod(file1, 0400));
+	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
+	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
+	EXPECT_EQ(0, test_chmod(file2, 0400));
+	EXPECT_EQ(0, test_chmod(dir1, 0700));
+	ASSERT_EQ(0, close(file1_fd));
+}
+
+TEST_F_FORK(layout1, chmod)
+{
+	int ruleset_fd, file1_fd;
+	const char *file1 = file1_s1d1;
+	const char *file2 = file2_s1d1;
+	const char *file3 = file1_s2d1;
+	const char *dir1 = dir_s1d1;
+	const char *dir2 = dir_s2d1;
+	const struct rule rules[] = {
+		{
+			.path = file1,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_CHMOD,
+		},
+		{
+			.path = file2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = dir1,
+			.access = ACCESS_RW,
+		},
+		{
+			.path = dir2,
+			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD,
+		},
+		{},
+	};
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
+	ASSERT_LE(0, ruleset_fd);
+	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file1_fd);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(0, test_chmod(file1, 0400));
+	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
+	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
+	EXPECT_EQ(EACCES, test_chmod(file2, 0400));
+	EXPECT_EQ(EACCES, test_chmod(dir1, 0700));
+	/* set CHMOD right on dir will only affect its context not dir itself*/
+	EXPECT_EQ(0, test_chmod(file3, 0400));
+	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file3, 0400, 0));
+	EXPECT_EQ(EACCES, test_chmod(dir2, 0700));
+	EXPECT_EQ(EACCES, test_fchmodat(AT_FDCWD, dir2, 0700, 0));
+	ASSERT_EQ(0, close(file1_fd));
+}
+
+TEST_F_FORK(layout1, unhandled_chown)
+{
+	int ruleset_fd, file1_fd;
+	const char *file1 = file1_s1d1;
+	const char *file2 = file2_s1d1;
+	const char *dir1 = dir_s1d1;
+	const struct rule rules[] = {
+		{
+			.path = file1,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = dir1,
+			.access = ACCESS_RW,
+		},
+		{},
+	};
+	struct stat st;
+	uid_t uid;
+	gid_t gid;
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+	ASSERT_LE(0, ruleset_fd);
+	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file1_fd);
+	/*
+	 * there is no CAP_CHOWN when the testcases framework setup,
+	 * and we cannot assume the testcases are run as root, to make
+	 * sure {f}chown syscall won't fail, get the original uid/gid and
+	 * use them in test_{f}chown.
+	 */
+	ASSERT_EQ(0, stat(dir1, &st));
+	uid = st.st_uid;
+	gid = st.st_gid;
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(0, test_chown(file1, uid, gid));
+	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
+	EXPECT_EQ(0, test_lchown(file1, uid, gid));
+	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
+	EXPECT_EQ(0, test_chown(file2, uid, gid));
+	EXPECT_EQ(0, test_chown(dir1, uid, gid));
+	ASSERT_EQ(0, close(file1_fd));
+}
+
+TEST_F_FORK(layout1, chown)
+{
+	int ruleset_fd, file1_fd;
+	const char *file1 = file1_s1d1;
+	const char *file2 = file2_s1d1;
+	const char *file3 = file1_s2d1;
+	const char *dir1 = dir_s1d1;
+	const char *dir2 = dir_s2d1;
+	const struct rule rules[] = {
+		{
+			.path = file1,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_CHGRP,
+		},
+		{
+			.path = file2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = dir1,
+			.access = ACCESS_RW,
+		},
+		{
+			.path = dir2,
+			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP,
+		},
+		{},
+	};
+	struct stat st;
+	uid_t uid;
+	gid_t gid;
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP, rules);
+	ASSERT_LE(0, ruleset_fd);
+	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file1_fd);
+	/*
+	 * there is no CAP_CHOWN when the testcases framework setup,
+	 * and we cannot assume the testcases are run as root, to make
+	 * sure {f}chown syscall won't fail, get the original uid/gid and
+	 * use them in test_{f}chown.
+	 */
+	ASSERT_EQ(0, stat(dir1, &st));
+	uid = st.st_uid;
+	gid = st.st_gid;
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(0, test_chown(file1, uid, gid));
+	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
+	EXPECT_EQ(0, test_lchown(file1, uid, gid));
+	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
+	EXPECT_EQ(EACCES, test_chown(file2, uid, gid));
+	EXPECT_EQ(EACCES, test_chown(dir1, uid, gid));
+	/* set CHOWN right on dir will only affect its context not dir itself*/
+	EXPECT_EQ(0, test_chown(file3, uid, gid));
+	EXPECT_EQ(EACCES, test_chown(dir2, uid, gid));
+	ASSERT_EQ(0, close(file1_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.17.1

Re: [PATCH -next v2 4/6] landlock/selftests: add selftests for chmod and chown

[Günther Noack]

On Sat, Aug 27, 2022 at 07:12:13PM +0800, Xiu Jianfeng wrote:
> The following APIs are tested with simple scenarios.
> 1. chmod/fchmod/fchmodat;
> 2. chmod/fchmod/lchown/fchownat;
>
> The key point is that set these access rights on a directory but only for
> its content, not the directory itself. this scenario is covered.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 261 +++++++++++++++++++++
>  1 file changed, 261 insertions(+)
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index f513cd8d9d51..982cb824967c 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3272,6 +3272,267 @@ TEST_F_FORK(layout1, truncate)
>  	EXPECT_EQ(0, test_creat(file_in_dir_w));
>  }
>
> +/* Invokes chmod(2) and returns its errno or 0. */
> +static int test_chmod(const char *const path, mode_t mode)
> +{
> +	if (chmod(path, mode) < 0)
> +		return errno;
> +	return 0;
> +}

Nice, this is much simpler than in the last revision :)

> +
> +/* Invokes fchmod(2) and returns its errno or 0. */
> +static int test_fchmod(int fd, mode_t mode)
> +{
> +	if (fchmod(fd, mode) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes fchmodat(2) and returns its errno or 0. */
> +static int test_fchmodat(int dirfd, const char *path, mode_t mode, int flags)

Nitpick: Some of these functions have path arguments declared as

  const char *path

and others as

  const char *const path

-- would be nice to stay consistent.

> +{
> +	if (fchmodat(dirfd, path, mode, flags) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes chown(2) and returns its errno or 0. */
> +static int test_chown(const char *path, uid_t uid, gid_t gid)
> +{
> +	if (chown(path, uid, gid) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes fchown(2) and returns its errno or 0. */
> +static int test_fchown(int fd, uid_t uid, gid_t gid)
> +{
> +	if (fchown(fd, uid, gid) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes lchown(2) and returns its errno or 0. */
> +static int test_lchown(const char *path, uid_t uid, gid_t gid)
> +{
> +	if (lchown(path, uid, gid) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes fchownat(2) and returns its errno or 0. */
> +static int test_fchownat(int dirfd, const char *path,
> +			 uid_t uid, gid_t gid, int flags)
> +{
> +	if (fchownat(dirfd, path, uid, gid, flags) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +TEST_F_FORK(layout1, unhandled_chmod)
> +{
> +	int ruleset_fd, file1_fd;
> +	const char *file1 = file1_s1d1;
> +	const char *file2 = file2_s1d1;
> +	const char *dir1 = dir_s1d1;

I'd suggest to name these kinds of variables according to the rights
that are granted on these files and directories, for example as as
w_file and rw_file. (Then, when looking at the EXPECT_EQ checks below,
you don't need to jump back up to the start of the test to remember
which of the rights is being tested.)

Nitpick: The same remark as above about the 'const' modifier applies
here as well. The rest of the file uses "const char *const varname".

> +	const struct rule rules[] = {
> +		{
> +			.path = file1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = dir1,
> +			.access = ACCESS_RW,
> +		},
> +		{},
> +	};
> +
> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file1_fd);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, test_chmod(file1, 0400));
> +	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
> +	EXPECT_EQ(0, test_chmod(file2, 0400));
> +	EXPECT_EQ(0, test_chmod(dir1, 0700));
> +	ASSERT_EQ(0, close(file1_fd));
> +}
> +
> +TEST_F_FORK(layout1, chmod)
> +{
> +	int ruleset_fd, file1_fd;
> +	const char *file1 = file1_s1d1;
> +	const char *file2 = file2_s1d1;
> +	const char *file3 = file1_s2d1;
> +	const char *dir1 = dir_s1d1;
> +	const char *dir2 = dir_s2d1;
> +	const struct rule rules[] = {
> +		{
> +			.path = file1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_CHMOD,
> +		},
> +		{
> +			.path = file2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = dir1,
> +			.access = ACCESS_RW,
> +		},
> +		{
> +			.path = dir2,
> +			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD,
> +		},
> +		{},
> +	};
> +
> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file1_fd);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, test_chmod(file1, 0400));
> +	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
> +	EXPECT_EQ(EACCES, test_chmod(file2, 0400));
> +	EXPECT_EQ(EACCES, test_chmod(dir1, 0700));
> +	/* set CHMOD right on dir will only affect its context not dir itself*/
> +	EXPECT_EQ(0, test_chmod(file3, 0400));
> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file3, 0400, 0));
> +	EXPECT_EQ(EACCES, test_chmod(dir2, 0700));
> +	EXPECT_EQ(EACCES, test_fchmodat(AT_FDCWD, dir2, 0700, 0));
> +	ASSERT_EQ(0, close(file1_fd));
> +}
> +
> +TEST_F_FORK(layout1, unhandled_chown)
> +{
> +	int ruleset_fd, file1_fd;
> +	const char *file1 = file1_s1d1;
> +	const char *file2 = file2_s1d1;
> +	const char *dir1 = dir_s1d1;
> +	const struct rule rules[] = {
> +		{
> +			.path = file1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = dir1,
> +			.access = ACCESS_RW,
> +		},
> +		{},
> +	};
> +	struct stat st;
> +	uid_t uid;
> +	gid_t gid;
> +
> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file1_fd);
> +	/*
> +	 * there is no CAP_CHOWN when the testcases framework setup,
> +	 * and we cannot assume the testcases are run as root, to make
> +	 * sure {f}chown syscall won't fail, get the original uid/gid and
> +	 * use them in test_{f}chown.
> +	 */
> +	ASSERT_EQ(0, stat(dir1, &st));
> +	uid = st.st_uid;
> +	gid = st.st_gid;
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, test_chown(file1, uid, gid));
> +	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
> +	EXPECT_EQ(0, test_lchown(file1, uid, gid));
> +	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
> +	EXPECT_EQ(0, test_chown(file2, uid, gid));
> +	EXPECT_EQ(0, test_chown(dir1, uid, gid));
> +	ASSERT_EQ(0, close(file1_fd));
> +}
> +
> +TEST_F_FORK(layout1, chown)
> +{
> +	int ruleset_fd, file1_fd;
> +	const char *file1 = file1_s1d1;
> +	const char *file2 = file2_s1d1;
> +	const char *file3 = file1_s2d1;
> +	const char *dir1 = dir_s1d1;
> +	const char *dir2 = dir_s2d1;
> +	const struct rule rules[] = {
> +		{
> +			.path = file1,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_CHGRP,
> +		},
> +		{
> +			.path = file2,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = dir1,
> +			.access = ACCESS_RW,
> +		},
> +		{
> +			.path = dir2,
> +			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP,
> +		},
> +		{},
> +	};
> +	struct stat st;
> +	uid_t uid;
> +	gid_t gid;
> +
> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file1_fd);
> +	/*
> +	 * there is no CAP_CHOWN when the testcases framework setup,
> +	 * and we cannot assume the testcases are run as root, to make
> +	 * sure {f}chown syscall won't fail, get the original uid/gid and
> +	 * use them in test_{f}chown.
> +	 */
> +	ASSERT_EQ(0, stat(dir1, &st));
> +	uid = st.st_uid;
> +	gid = st.st_gid;
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	EXPECT_EQ(0, test_chown(file1, uid, gid));
> +	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
> +	EXPECT_EQ(0, test_lchown(file1, uid, gid));
> +	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
> +	EXPECT_EQ(EACCES, test_chown(file2, uid, gid));
> +	EXPECT_EQ(EACCES, test_chown(dir1, uid, gid));
> +	/* set CHOWN right on dir will only affect its context not dir itself*/
> +	EXPECT_EQ(0, test_chown(file3, uid, gid));
> +	EXPECT_EQ(EACCES, test_chown(dir2, uid, gid));
> +	ASSERT_EQ(0, close(file1_fd));
> +}
> +
>  /* clang-format off */
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
> --
> 2.17.1
>

--

Re: [PATCH -next v2 4/6] landlock/selftests: add selftests for chmod and chown

[xiujianfeng]

Hi,

在 2022/8/28 1:48, Günther Noack 写道:
> On Sat, Aug 27, 2022 at 07:12:13PM +0800, Xiu Jianfeng wrote:
>> The following APIs are tested with simple scenarios.
>> 1. chmod/fchmod/fchmodat;
>> 2. chmod/fchmod/lchown/fchownat;
>>
>> The key point is that set these access rights on a directory but only for
>> its content, not the directory itself. this scenario is covered.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   tools/testing/selftests/landlock/fs_test.c | 261 +++++++++++++++++++++
>>   1 file changed, 261 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index f513cd8d9d51..982cb824967c 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -3272,6 +3272,267 @@ TEST_F_FORK(layout1, truncate)
>>   	EXPECT_EQ(0, test_creat(file_in_dir_w));
>>   }
>>
>> +/* Invokes chmod(2) and returns its errno or 0. */
>> +static int test_chmod(const char *const path, mode_t mode)
>> +{
>> +	if (chmod(path, mode) < 0)
>> +		return errno;
>> +	return 0;
>> +}
> 
> Nice, this is much simpler than in the last revision :)
> 
>> +
>> +/* Invokes fchmod(2) and returns its errno or 0. */
>> +static int test_fchmod(int fd, mode_t mode)
>> +{
>> +	if (fchmod(fd, mode) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +/* Invokes fchmodat(2) and returns its errno or 0. */
>> +static int test_fchmodat(int dirfd, const char *path, mode_t mode, int flags)
> 
> Nitpick: Some of these functions have path arguments declared as
> 
>    const char *path
> 
> and others as
> 
>    const char *const path
> 
> -- would be nice to stay consistent.

Thanks, const char *const path would be good to me.

> 
>> +{
>> +	if (fchmodat(dirfd, path, mode, flags) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +/* Invokes chown(2) and returns its errno or 0. */
>> +static int test_chown(const char *path, uid_t uid, gid_t gid)
>> +{
>> +	if (chown(path, uid, gid) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +/* Invokes fchown(2) and returns its errno or 0. */
>> +static int test_fchown(int fd, uid_t uid, gid_t gid)
>> +{
>> +	if (fchown(fd, uid, gid) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +/* Invokes lchown(2) and returns its errno or 0. */
>> +static int test_lchown(const char *path, uid_t uid, gid_t gid)
>> +{
>> +	if (lchown(path, uid, gid) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +/* Invokes fchownat(2) and returns its errno or 0. */
>> +static int test_fchownat(int dirfd, const char *path,
>> +			 uid_t uid, gid_t gid, int flags)
>> +{
>> +	if (fchownat(dirfd, path, uid, gid, flags) < 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chmod)
>> +{
>> +	int ruleset_fd, file1_fd;
>> +	const char *file1 = file1_s1d1;
>> +	const char *file2 = file2_s1d1;
>> +	const char *dir1 = dir_s1d1;
> 
> I'd suggest to name these kinds of variables according to the rights
> that are granted on these files and directories, for example as as
> w_file and rw_file. (Then, when looking at the EXPECT_EQ checks below,
> you don't need to jump back up to the start of the test to remember
> which of the rights is being tested.)
> 
> Nitpick: The same remark as above about the 'const' modifier applies
> here as well. The rest of the file uses "const char *const varname".

Good point, will do it in v3.
> 
>> +	const struct rule rules[] = {
>> +		{
>> +			.path = file1,
>> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = file2,
>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = dir1,
>> +			.access = ACCESS_RW,
>> +		},
>> +		{},
>> +	};
>> +
>> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> +	ASSERT_LE(0, file1_fd);
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	EXPECT_EQ(0, test_chmod(file1, 0400));
>> +	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
>> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
>> +	EXPECT_EQ(0, test_chmod(file2, 0400));
>> +	EXPECT_EQ(0, test_chmod(dir1, 0700));
>> +	ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, chmod)
>> +{
>> +	int ruleset_fd, file1_fd;
>> +	const char *file1 = file1_s1d1;
>> +	const char *file2 = file2_s1d1;
>> +	const char *file3 = file1_s2d1;
>> +	const char *dir1 = dir_s1d1;
>> +	const char *dir2 = dir_s2d1;
>> +	const struct rule rules[] = {
>> +		{
>> +			.path = file1,
>> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
>> +				  LANDLOCK_ACCESS_FS_CHMOD,
>> +		},
>> +		{
>> +			.path = file2,
>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = dir1,
>> +			.access = ACCESS_RW,
>> +		},
>> +		{
>> +			.path = dir2,
>> +			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD,
>> +		},
>> +		{},
>> +	};
>> +
>> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> +	ASSERT_LE(0, file1_fd);
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	EXPECT_EQ(0, test_chmod(file1, 0400));
>> +	EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
>> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
>> +	EXPECT_EQ(EACCES, test_chmod(file2, 0400));
>> +	EXPECT_EQ(EACCES, test_chmod(dir1, 0700));
>> +	/* set CHMOD right on dir will only affect its context not dir itself*/
>> +	EXPECT_EQ(0, test_chmod(file3, 0400));
>> +	EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file3, 0400, 0));
>> +	EXPECT_EQ(EACCES, test_chmod(dir2, 0700));
>> +	EXPECT_EQ(EACCES, test_fchmodat(AT_FDCWD, dir2, 0700, 0));
>> +	ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chown)
>> +{
>> +	int ruleset_fd, file1_fd;
>> +	const char *file1 = file1_s1d1;
>> +	const char *file2 = file2_s1d1;
>> +	const char *dir1 = dir_s1d1;
>> +	const struct rule rules[] = {
>> +		{
>> +			.path = file1,
>> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = file2,
>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = dir1,
>> +			.access = ACCESS_RW,
>> +		},
>> +		{},
>> +	};
>> +	struct stat st;
>> +	uid_t uid;
>> +	gid_t gid;
>> +
>> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> +	ASSERT_LE(0, file1_fd);
>> +	/*
>> +	 * there is no CAP_CHOWN when the testcases framework setup,
>> +	 * and we cannot assume the testcases are run as root, to make
>> +	 * sure {f}chown syscall won't fail, get the original uid/gid and
>> +	 * use them in test_{f}chown.
>> +	 */
>> +	ASSERT_EQ(0, stat(dir1, &st));
>> +	uid = st.st_uid;
>> +	gid = st.st_gid;
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	EXPECT_EQ(0, test_chown(file1, uid, gid));
>> +	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
>> +	EXPECT_EQ(0, test_lchown(file1, uid, gid));
>> +	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
>> +	EXPECT_EQ(0, test_chown(file2, uid, gid));
>> +	EXPECT_EQ(0, test_chown(dir1, uid, gid));
>> +	ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, chown)
>> +{
>> +	int ruleset_fd, file1_fd;
>> +	const char *file1 = file1_s1d1;
>> +	const char *file2 = file2_s1d1;
>> +	const char *file3 = file1_s2d1;
>> +	const char *dir1 = dir_s1d1;
>> +	const char *dir2 = dir_s2d1;
>> +	const struct rule rules[] = {
>> +		{
>> +			.path = file1,
>> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE |
>> +				  LANDLOCK_ACCESS_FS_CHGRP,
>> +		},
>> +		{
>> +			.path = file2,
>> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
>> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
>> +		},
>> +		{
>> +			.path = dir1,
>> +			.access = ACCESS_RW,
>> +		},
>> +		{
>> +			.path = dir2,
>> +			.access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP,
>> +		},
>> +		{},
>> +	};
>> +	struct stat st;
>> +	uid_t uid;
>> +	gid_t gid;
>> +
>> +	ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP, rules);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> +	ASSERT_LE(0, file1_fd);
>> +	/*
>> +	 * there is no CAP_CHOWN when the testcases framework setup,
>> +	 * and we cannot assume the testcases are run as root, to make
>> +	 * sure {f}chown syscall won't fail, get the original uid/gid and
>> +	 * use them in test_{f}chown.
>> +	 */
>> +	ASSERT_EQ(0, stat(dir1, &st));
>> +	uid = st.st_uid;
>> +	gid = st.st_gid;
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	EXPECT_EQ(0, test_chown(file1, uid, gid));
>> +	EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
>> +	EXPECT_EQ(0, test_lchown(file1, uid, gid));
>> +	EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
>> +	EXPECT_EQ(EACCES, test_chown(file2, uid, gid));
>> +	EXPECT_EQ(EACCES, test_chown(dir1, uid, gid));
>> +	/* set CHOWN right on dir will only affect its context not dir itself*/
>> +	EXPECT_EQ(0, test_chown(file3, uid, gid));
>> +	EXPECT_EQ(EACCES, test_chown(dir2, uid, gid));
>> +	ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>>   /* clang-format off */
>>   FIXTURE(layout1_bind) {};
>>   /* clang-format on */
>> --
>> 2.17.1
>>
> 
> --
> .
> 

[PATCH -next v2 5/6] landlock/samples: add chmod and chown support

[Xiu Jianfeng]

update landlock sample to support the new flags
LANDLOCK_ACCESS_FS_{CHMOD, CHGRP}

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 samples/landlock/sandboxer.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 771b6b10d519..639ec39ebd0a 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -77,7 +77,9 @@ static int parse_path(char *env_path, const char ***const path_list)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_CHMOD | \
+	LANDLOCK_ACCESS_FS_CHGRP)
 
 /* clang-format on */
 
@@ -162,7 +164,9 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
 	LANDLOCK_ACCESS_FS_REFER | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_CHMOD | \
+	LANDLOCK_ACCESS_FS_CHGRP)
 
 /* clang-format on */
 
@@ -233,6 +237,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	case 2:
 		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+		__attribute__((fallthrough));
+	case 3:
+		/* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} for ABI < 4 */
+		ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
+						    LANDLOCK_ACCESS_FS_CHGRP);
 	}
 	access_fs_ro &= ruleset_attr.handled_access_fs;
 	access_fs_rw &= ruleset_attr.handled_access_fs;
-- 
2.17.1

[PATCH -next v2 6/6] landlock: update chmod and chown support in document

[Xiu Jianfeng]

update LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} support and add abi change
in the document.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 Documentation/userspace-api/landlock.rst | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 2509c2fbf98f..0e97a7998fa1 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -61,7 +61,9 @@ the need to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_FS_MAKE_BLOCK |
             LANDLOCK_ACCESS_FS_MAKE_SYM |
             LANDLOCK_ACCESS_FS_REFER |
-            LANDLOCK_ACCESS_FS_TRUNCATE,
+            LANDLOCK_ACCESS_FS_TRUNCATE |
+            LANDLOCK_ACCESS_FS_CHMOD |
+            LANDLOCK_ACCESS_FS_CHGRP
     };
 
 Because we may not know on which kernel version an application will be
@@ -90,6 +92,11 @@ the ABI.
     case 2:
             /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
             ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+            __attribute__((fallthrough));
+    case 3:
+            /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} for ABI < 4 */
+            ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
+                                                LANDLOCK_ACCESS_FS_CHGRP);
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
-- 
2.17.1

Re: [PATCH -next v2 6/6] landlock: update chmod and chown support in document

[Günther Noack]

On Sat, Aug 27, 2022 at 07:12:15PM +0800, Xiu Jianfeng wrote:
> update LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} support and add abi change
> in the document.
>
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  Documentation/userspace-api/landlock.rst | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 2509c2fbf98f..0e97a7998fa1 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -61,7 +61,9 @@ the need to be explicit about the denied-by-default access rights.
>              LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>              LANDLOCK_ACCESS_FS_MAKE_SYM |
>              LANDLOCK_ACCESS_FS_REFER |
> -            LANDLOCK_ACCESS_FS_TRUNCATE,
> +            LANDLOCK_ACCESS_FS_TRUNCATE |
> +            LANDLOCK_ACCESS_FS_CHMOD |
> +            LANDLOCK_ACCESS_FS_CHGRP
>      };
>
>  Because we may not know on which kernel version an application will be
> @@ -90,6 +92,11 @@ the ABI.
>      case 2:
>              /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
>              ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> +            __attribute__((fallthrough));
> +    case 3:
> +            /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} for ABI < 4 */
> +            ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
> +                                                LANDLOCK_ACCESS_FS_CHGRP);
>      }
>
>  This enables to create an inclusive ruleset that will contain our rules.

There is a sentence just above this code example that neesd updating as well.

--

Re: [PATCH -next v2 6/6] landlock: update chmod and chown support in document

[xiujianfeng]

Hi,

在 2022/8/28 1:28, Günther Noack 写道:
> On Sat, Aug 27, 2022 at 07:12:15PM +0800, Xiu Jianfeng wrote:
>> update LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} support and add abi change
>> in the document.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>   Documentation/userspace-api/landlock.rst | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
>> index 2509c2fbf98f..0e97a7998fa1 100644
>> --- a/Documentation/userspace-api/landlock.rst
>> +++ b/Documentation/userspace-api/landlock.rst
>> @@ -61,7 +61,9 @@ the need to be explicit about the denied-by-default access rights.
>>               LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>>               LANDLOCK_ACCESS_FS_MAKE_SYM |
>>               LANDLOCK_ACCESS_FS_REFER |
>> -            LANDLOCK_ACCESS_FS_TRUNCATE,
>> +            LANDLOCK_ACCESS_FS_TRUNCATE |
>> +            LANDLOCK_ACCESS_FS_CHMOD |
>> +            LANDLOCK_ACCESS_FS_CHGRP
>>       };
>>
>>   Because we may not know on which kernel version an application will be
>> @@ -90,6 +92,11 @@ the ABI.
>>       case 2:
>>               /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
>>               ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
>> +            __attribute__((fallthrough));
>> +    case 3:
>> +            /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHGRP} for ABI < 4 */
>> +            ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
>> +                                                LANDLOCK_ACCESS_FS_CHGRP);
>>       }
>>
>>   This enables to create an inclusive ruleset that will contain our rules.
> 
> There is a sentence just above this code example that neesd updating as well.
> 

Get it, it's the follwing sentence, thanks.
...
Let's check if we should
remove the `LANDLOCK_ACCESS_FS_REFER` or `LANDLOCK_ACCESS_FS_TRUNCATE` 
access
rights, which are only supported starting with the second and third 
version of
the ABI.
...

> --
> .
> 

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[Mickaël Salaün]

Please add a description and include links to the previous versions.

On 27/08/2022 13:12, Xiu Jianfeng wrote:
> v2:
>   * abstract walk_to_visible_parent() helper
>   * chmod and chown rights only take affect on directory's context
>   * add testcase for fchmodat/lchown/fchownat
>   * fix other review issues
> 
> Xiu Jianfeng (6):
>    landlock: expand access_mask_t to u32 type
>    landlock: abstract walk_to_visible_parent() helper
>    landlock: add chmod and chown support
>    landlock/selftests: add selftests for chmod and chown
>    landlock/samples: add chmod and chown support
>    landlock: update chmod and chown support in document
> 
>   Documentation/userspace-api/landlock.rst     |   9 +-
>   include/uapi/linux/landlock.h                |  10 +-
>   samples/landlock/sandboxer.c                 |  13 +-
>   security/landlock/fs.c                       | 110 ++++++--
>   security/landlock/limits.h                   |   2 +-
>   security/landlock/ruleset.h                  |   2 +-
>   security/landlock/syscalls.c                 |   2 +-
>   tools/testing/selftests/landlock/base_test.c |   2 +-
>   tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
>   9 files changed, 386 insertions(+), 31 deletions(-)
> 

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[xiujianfeng]

Hi Mickael,

Sorry about the long silence on this work, As we known this work depends
on another work about changing argument from struct dentry to struct
path for some attr/xattr related lsm hooks, I'm stuck with this thing,
because IMA/EVM is a special security module which is not LSM-based
currently, and severely coupled with the file system. so I am waiting
for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
be ready, I think it can make my work more easy. you can find
Roberto'work here,
https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huaweicloud.com/

Any good idea are welcome, thanks.


On 2022/8/27 19:12, Xiu Jianfeng wrote:
> v2:
>  * abstract walk_to_visible_parent() helper
>  * chmod and chown rights only take affect on directory's context
>  * add testcase for fchmodat/lchown/fchownat
>  * fix other review issues
> 
> Xiu Jianfeng (6):
>   landlock: expand access_mask_t to u32 type
>   landlock: abstract walk_to_visible_parent() helper
>   landlock: add chmod and chown support
>   landlock/selftests: add selftests for chmod and chown
>   landlock/samples: add chmod and chown support
>   landlock: update chmod and chown support in document
> 
>  Documentation/userspace-api/landlock.rst     |   9 +-
>  include/uapi/linux/landlock.h                |  10 +-
>  samples/landlock/sandboxer.c                 |  13 +-
>  security/landlock/fs.c                       | 110 ++++++--
>  security/landlock/limits.h                   |   2 +-
>  security/landlock/ruleset.h                  |   2 +-
>  security/landlock/syscalls.c                 |   2 +-
>  tools/testing/selftests/landlock/base_test.c |   2 +-
>  tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
>  9 files changed, 386 insertions(+), 31 deletions(-)
> 

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[Mickaël Salaün]

On 18/04/2023 12:53, xiujianfeng wrote:
> Hi Mickael,
> 
> Sorry about the long silence on this work, As we known this work depends
> on another work about changing argument from struct dentry to struct
> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
> because IMA/EVM is a special security module which is not LSM-based
> currently, and severely coupled with the file system. so I am waiting
> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
> be ready, I think it can make my work more easy. you can find
> Roberto'work here,
> https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huaweicloud.com/
> 
> Any good idea are welcome, thanks.

Thanks for the update Xiu.

Which part would be needed from Roberto's patch series?


> 
> 
> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>> v2:
>>   * abstract walk_to_visible_parent() helper
>>   * chmod and chown rights only take affect on directory's context
>>   * add testcase for fchmodat/lchown/fchownat
>>   * fix other review issues
>>
>> Xiu Jianfeng (6):
>>    landlock: expand access_mask_t to u32 type
>>    landlock: abstract walk_to_visible_parent() helper
>>    landlock: add chmod and chown support
>>    landlock/selftests: add selftests for chmod and chown
>>    landlock/samples: add chmod and chown support
>>    landlock: update chmod and chown support in document
>>
>>   Documentation/userspace-api/landlock.rst     |   9 +-
>>   include/uapi/linux/landlock.h                |  10 +-
>>   samples/landlock/sandboxer.c                 |  13 +-
>>   security/landlock/fs.c                       | 110 ++++++--
>>   security/landlock/limits.h                   |   2 +-
>>   security/landlock/ruleset.h                  |   2 +-
>>   security/landlock/syscalls.c                 |   2 +-
>>   tools/testing/selftests/landlock/base_test.c |   2 +-
>>   tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
>>   9 files changed, 386 insertions(+), 31 deletions(-)
>>

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[xiujianfeng]

On 2023/4/21 1:40, Mickaël Salaün wrote:
> 
> On 18/04/2023 12:53, xiujianfeng wrote:
>> Hi Mickael,
>>
>> Sorry about the long silence on this work, As we known this work depends
>> on another work about changing argument from struct dentry to struct
>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>> because IMA/EVM is a special security module which is not LSM-based
>> currently, and severely coupled with the file system. so I am waiting
>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>> be ready, I think it can make my work more easy. you can find
>> Roberto'work here,
>> https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huaweicloud.com/
>>
>> Any good idea are welcome, thanks.
> 
> Thanks for the update Xiu.
> 
> Which part would be needed from Roberto's patch series?
> 
As we discussed before, the two access rights that need to be added and
their usage is as below:
LANDLOCK_ACCESS_FS_WRITE_METADATA controls
1.inode_setattr
2.inode_setxattr
3.inode_removexattr
4.inode_set_acl
5.inode_remove_acl
LANDLOCK_ACCESS_FS_READ_METADATA controls
1.inode_getattr
2.inode_get_acl
3.inode_getxattr
4.inode_listxattr

all these APIs should be changed to use struct path instead of dentry,
and then several vfs APIs as follows are invovled:
notify_change,
__vfs_setxattr_locked,
__vfs_removexattr_locked,
__vfs_setxattr_noperm
vfs_set_acl
vfs_remove_acl
vfs_getxattr
vfs_listxattr
vfs_get_acl
and also include some LSM hooks such as inode_post_setxattr and
inode_setsecctx.

Since the original places where pass dentry to security_inode_xxx may
not have any struct path, we have to pass it from the top caller, so
this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
nfsd, overlayfs...).

Other LSMs such as selinux, smack can be easy to refator because they
are LSM-based, and if VFS passes path to security_inode_xxx and they can
just use path->dentry instead inside they own modules.

AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
the file system. To make things worse, there is a recursive dependency
situation during the update of extended attribute which happen as follows:

__vfs_setxattr_noperm
  => security_inode_post_setxattr
    => evm_inode_post_setxattr
      => evm_update_evmxattr
=> __vfs_setxattr_noperm

To change the argument of __vfs_setxattr_noperm from a dentry to the
path structure, the two EVM functions would have to be altered as well.
However, evm_update_evmxattr is called by 3 other EVM functions who
lives in the very heart of the complicated EVM framework. Any change to
them would cause a nasty chain reaction in EVM and, as IMA would trigger
EVM directly, in IMA as well.

There is another callchain as follow:
ima_appraise_measurement
  =>evm_verifyxattr
    =>evm_verifyxattr
      =>evm_verify_hmac
	=>evm_calc_hash
	   =>evm_calc_hmac_or_hash
	     =>vfs_getxattr
Passing struct path into vfs_getxattr() would also affect this
callchain. Currently ima_appraise_measurment accepts a struct file, and
dentry is generated from file_dentry(file) in order to mitigate a
deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
&file->f_path is passed through this callchain, and someone wants the
dentry, it will be using file->f_path.dentry, which is different from
file_dentry(file). In the overlayfs scenario, may this cause an issue?

The patchset of moving IMA and EVM into the LSM infrastructe would be
helpfull but still can not completely resolve this situation. more
refactor would be needed in EVM. That's all that's happening right now.

> 
>>
>>
>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>> v2:
>>>   * abstract walk_to_visible_parent() helper
>>>   * chmod and chown rights only take affect on directory's context
>>>   * add testcase for fchmodat/lchown/fchownat
>>>   * fix other review issues
>>>
>>> Xiu Jianfeng (6):
>>>    landlock: expand access_mask_t to u32 type
>>>    landlock: abstract walk_to_visible_parent() helper
>>>    landlock: add chmod and chown support
>>>    landlock/selftests: add selftests for chmod and chown
>>>    landlock/samples: add chmod and chown support
>>>    landlock: update chmod and chown support in document
>>>
>>>   Documentation/userspace-api/landlock.rst     |   9 +-
>>>   include/uapi/linux/landlock.h                |  10 +-
>>>   samples/landlock/sandboxer.c                 |  13 +-
>>>   security/landlock/fs.c                       | 110 ++++++--
>>>   security/landlock/limits.h                   |   2 +-
>>>   security/landlock/ruleset.h                  |   2 +-
>>>   security/landlock/syscalls.c                 |   2 +-
>>>   tools/testing/selftests/landlock/base_test.c |   2 +-
>>>   tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
>>>   9 files changed, 386 insertions(+), 31 deletions(-)
>>>

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[Mickaël Salaün]

On 24/04/2023 10:52, xiujianfeng wrote:
> 
> 
> On 2023/4/21 1:40, Mickaël Salaün wrote:
>>
>> On 18/04/2023 12:53, xiujianfeng wrote:
>>> Hi Mickael,
>>>
>>> Sorry about the long silence on this work, As we known this work depends
>>> on another work about changing argument from struct dentry to struct
>>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>>> because IMA/EVM is a special security module which is not LSM-based
>>> currently, and severely coupled with the file system. so I am waiting
>>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>>> be ready, I think it can make my work more easy. you can find
>>> Roberto'work here,
>>> https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huaweicloud.com/
>>>
>>> Any good idea are welcome, thanks.
>>
>> Thanks for the update Xiu.
>>
>> Which part would be needed from Roberto's patch series?
>>
> As we discussed before, the two access rights that need to be added and
> their usage is as below:
> LANDLOCK_ACCESS_FS_WRITE_METADATA controls
> 1.inode_setattr
> 2.inode_setxattr
> 3.inode_removexattr
> 4.inode_set_acl
> 5.inode_remove_acl
> LANDLOCK_ACCESS_FS_READ_METADATA controls
> 1.inode_getattr
> 2.inode_get_acl
> 3.inode_getxattr
> 4.inode_listxattr
> 
> all these APIs should be changed to use struct path instead of dentry,
> and then several vfs APIs as follows are invovled:
> notify_change,
> __vfs_setxattr_locked,
> __vfs_removexattr_locked,
> __vfs_setxattr_noperm
> vfs_set_acl
> vfs_remove_acl
> vfs_getxattr
> vfs_listxattr
> vfs_get_acl
> and also include some LSM hooks such as inode_post_setxattr and
> inode_setsecctx.
> 
> Since the original places where pass dentry to security_inode_xxx may
> not have any struct path, we have to pass it from the top caller, so
> this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
> nfsd, overlayfs...).
> 
> Other LSMs such as selinux, smack can be easy to refator because they
> are LSM-based, and if VFS passes path to security_inode_xxx and they can
> just use path->dentry instead inside they own modules.
> 
> AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
> the file system. To make things worse, there is a recursive dependency
> situation during the update of extended attribute which happen as follows:
> 
> __vfs_setxattr_noperm
>    => security_inode_post_setxattr
>      => evm_inode_post_setxattr
>        => evm_update_evmxattr
> => __vfs_setxattr_noperm
> 
> To change the argument of __vfs_setxattr_noperm from a dentry to the
> path structure, the two EVM functions would have to be altered as well.
> However, evm_update_evmxattr is called by 3 other EVM functions who
> lives in the very heart of the complicated EVM framework. Any change to
> them would cause a nasty chain reaction in EVM and, as IMA would trigger
> EVM directly, in IMA as well.
> 
> There is another callchain as follow:
> ima_appraise_measurement
>    =>evm_verifyxattr
>      =>evm_verifyxattr
>        =>evm_verify_hmac
> 	=>evm_calc_hash
> 	   =>evm_calc_hmac_or_hash
> 	     =>vfs_getxattr
> Passing struct path into vfs_getxattr() would also affect this
> callchain. Currently ima_appraise_measurment accepts a struct file, and
> dentry is generated from file_dentry(file) in order to mitigate a
> deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
> &file->f_path is passed through this callchain, and someone wants the
> dentry, it will be using file->f_path.dentry, which is different from
> file_dentry(file). In the overlayfs scenario, may this cause an issue?

I might be OK, but this need to be tested.

> 
> The patchset of moving IMA and EVM into the LSM infrastructe would be
> helpfull but still can not completely resolve this situation. more
> refactor would be needed in EVM. That's all that's happening right now.

OK, thanks for the detailed explanation!

I guess you could start with easier hooks (e.g. inode_getattr and 
inode_setattr) to see if there is potentially other implications, and 
incrementally build on that.


> 
>>
>>>
>>>
>>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>>> v2:
>>>>    * abstract walk_to_visible_parent() helper
>>>>    * chmod and chown rights only take affect on directory's context
>>>>    * add testcase for fchmodat/lchown/fchownat
>>>>    * fix other review issues
>>>>
>>>> Xiu Jianfeng (6):
>>>>     landlock: expand access_mask_t to u32 type
>>>>     landlock: abstract walk_to_visible_parent() helper
>>>>     landlock: add chmod and chown support
>>>>     landlock/selftests: add selftests for chmod and chown
>>>>     landlock/samples: add chmod and chown support
>>>>     landlock: update chmod and chown support in document
>>>>
>>>>    Documentation/userspace-api/landlock.rst     |   9 +-
>>>>    include/uapi/linux/landlock.h                |  10 +-
>>>>    samples/landlock/sandboxer.c                 |  13 +-
>>>>    security/landlock/fs.c                       | 110 ++++++--
>>>>    security/landlock/limits.h                   |   2 +-
>>>>    security/landlock/ruleset.h                  |   2 +-
>>>>    security/landlock/syscalls.c                 |   2 +-
>>>>    tools/testing/selftests/landlock/base_test.c |   2 +-
>>>>    tools/testing/selftests/landlock/fs_test.c   | 267 ++++++++++++++++++-
>>>>    9 files changed, 386 insertions(+), 31 deletions(-)
>>>>

Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[xiujianfeng]

On 2023/4/26 21:58, Mickaël Salaün wrote:
> 
> 
> On 24/04/2023 10:52, xiujianfeng wrote:
>>
>>
>> On 2023/4/21 1:40, Mickaël Salaün wrote:
>>>
>>> On 18/04/2023 12:53, xiujianfeng wrote:
>>>> Hi Mickael,
>>>>
>>>> Sorry about the long silence on this work, As we known this work
>>>> depends
>>>> on another work about changing argument from struct dentry to struct
>>>> path for some attr/xattr related lsm hooks, I'm stuck with this thing,
>>>> because IMA/EVM is a special security module which is not LSM-based
>>>> currently, and severely coupled with the file system. so I am waiting
>>>> for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to
>>>> be ready, I think it can make my work more easy. you can find
>>>> Roberto'work here,
>>>> https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huaweicloud.com/
>>>>
>>>> Any good idea are welcome, thanks.
>>>
>>> Thanks for the update Xiu.
>>>
>>> Which part would be needed from Roberto's patch series?
>>>
>> As we discussed before, the two access rights that need to be added and
>> their usage is as below:
>> LANDLOCK_ACCESS_FS_WRITE_METADATA controls
>> 1.inode_setattr
>> 2.inode_setxattr
>> 3.inode_removexattr
>> 4.inode_set_acl
>> 5.inode_remove_acl
>> LANDLOCK_ACCESS_FS_READ_METADATA controls
>> 1.inode_getattr
>> 2.inode_get_acl
>> 3.inode_getxattr
>> 4.inode_listxattr
>>
>> all these APIs should be changed to use struct path instead of dentry,
>> and then several vfs APIs as follows are invovled:
>> notify_change,
>> __vfs_setxattr_locked,
>> __vfs_removexattr_locked,
>> __vfs_setxattr_noperm
>> vfs_set_acl
>> vfs_remove_acl
>> vfs_getxattr
>> vfs_listxattr
>> vfs_get_acl
>> and also include some LSM hooks such as inode_post_setxattr and
>> inode_setsecctx.
>>
>> Since the original places where pass dentry to security_inode_xxx may
>> not have any struct path, we have to pass it from the top caller, so
>> this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd,
>> nfsd, overlayfs...).
>>
>> Other LSMs such as selinux, smack can be easy to refator because they
>> are LSM-based, and if VFS passes path to security_inode_xxx and they can
>> just use path->dentry instead inside they own modules.
>>
>> AS for IMA/EVM, unfortunately they are not LSM-based and coupled with
>> the file system. To make things worse, there is a recursive dependency
>> situation during the update of extended attribute which happen as
>> follows:
>>
>> __vfs_setxattr_noperm
>>    => security_inode_post_setxattr
>>      => evm_inode_post_setxattr
>>        => evm_update_evmxattr
>> => __vfs_setxattr_noperm
>>
>> To change the argument of __vfs_setxattr_noperm from a dentry to the
>> path structure, the two EVM functions would have to be altered as well.
>> However, evm_update_evmxattr is called by 3 other EVM functions who
>> lives in the very heart of the complicated EVM framework. Any change to
>> them would cause a nasty chain reaction in EVM and, as IMA would trigger
>> EVM directly, in IMA as well.
>>
>> There is another callchain as follow:
>> ima_appraise_measurement
>>    =>evm_verifyxattr
>>      =>evm_verifyxattr
>>        =>evm_verify_hmac
>>     =>evm_calc_hash
>>        =>evm_calc_hmac_or_hash
>>          =>vfs_getxattr
>> Passing struct path into vfs_getxattr() would also affect this
>> callchain. Currently ima_appraise_measurment accepts a struct file, and
>> dentry is generated from file_dentry(file) in order to mitigate a
>> deadlock issue involving overlayfs(commit e71b9dff0634ed). Once
>> &file->f_path is passed through this callchain, and someone wants the
>> dentry, it will be using file->f_path.dentry, which is different from
>> file_dentry(file). In the overlayfs scenario, may this cause an issue?
> 
> I might be OK, but this need to be tested.
> 
>>
>> The patchset of moving IMA and EVM into the LSM infrastructe would be
>> helpfull but still can not completely resolve this situation. more
>> refactor would be needed in EVM. That's all that's happening right now.
> 
> OK, thanks for the detailed explanation!
> 
> I guess you could start with easier hooks (e.g. inode_getattr and
> inode_setattr) to see if there is potentially other implications, and
> incrementally build on that.

I was thinking to get everything done at once, but it looks like we are
having some troubles, so it's good advice to separate it into small
works and upstream them on by one, Thanks.

Currently inode_getattr already takes a struct path argument in the
mainline, I would send the patches about inode_setattr refactor and
discuss the problem of file_dentry() on that thread.

> 
> 
>>
>>>
>>>>
>>>>
>>>> On 2022/8/27 19:12, Xiu Jianfeng wrote:
>>>>> v2:
>>>>>    * abstract walk_to_visible_parent() helper
>>>>>    * chmod and chown rights only take affect on directory's context
>>>>>    * add testcase for fchmodat/lchown/fchownat
>>>>>    * fix other review issues
>>>>>
>>>>> Xiu Jianfeng (6):
>>>>>     landlock: expand access_mask_t to u32 type
>>>>>     landlock: abstract walk_to_visible_parent() helper
>>>>>     landlock: add chmod and chown support
>>>>>     landlock/selftests: add selftests for chmod and chown
>>>>>     landlock/samples: add chmod and chown support
>>>>>     landlock: update chmod and chown support in document
>>>>>
>>>>>    Documentation/userspace-api/landlock.rst     |   9 +-
>>>>>    include/uapi/linux/landlock.h                |  10 +-
>>>>>    samples/landlock/sandboxer.c                 |  13 +-
>>>>>    security/landlock/fs.c                       | 110 ++++++--
>>>>>    security/landlock/limits.h                   |   2 +-
>>>>>    security/landlock/ruleset.h                  |   2 +-
>>>>>    security/landlock/syscalls.c                 |   2 +-
>>>>>    tools/testing/selftests/landlock/base_test.c |   2 +-
>>>>>    tools/testing/selftests/landlock/fs_test.c   | 267
>>>>> ++++++++++++++++++-
>>>>>    9 files changed, 386 insertions(+), 31 deletions(-)
>>>>>