[v6,2/5] landlock: Support file truncation

Commit Message

Günther Noack Sept. 8, 2022, 7:58 p.m. UTC

Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.

This flag hooks into the path_truncate LSM hook and covers file
truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
well as creat().

This change also increments the Landlock ABI version, updates
corresponding selftests, and updates code documentation to document
the flag.

The following operations are restricted:

open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
implicitly truncated as part of the open() (e.g. using O_TRUNC).

Notable special cases:
* open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
* open() with O_TRUNC does *not* need the TRUNCATE right when it
  creates a new file.

truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
right.

ftruncate() (on a file): requires that the file had the TRUNCATE right
when it was previously opened.

Signed-off-by: Günther Noack <[e-mail]>
---
 include/uapi/linux/landlock.h                | 18 ++--
 security/landlock/fs.c                       | 88 +++++++++++++++++++-
 security/landlock/fs.h                       | 18 ++++
 security/landlock/limits.h                   |  2 +-
 security/landlock/setup.c                    |  1 +
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  7 +-
 8 files changed, 124 insertions(+), 14 deletions(-)

Comments

Mickaël Salaün Sept. 9, 2022, 1:51 p.m. UTC | #1

On 08/09/2022 21:58, Günther Noack wrote:
> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> 
> This flag hooks into the path_truncate LSM hook and covers file
> truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
> well as creat().
> 
> This change also increments the Landlock ABI version, updates
> corresponding selftests, and updates code documentation to document
> the flag.
> 
> The following operations are restricted:
> 
> open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> implicitly truncated as part of the open() (e.g. using O_TRUNC).
> 
> Notable special cases:
> * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> * open() with O_TRUNC does *not* need the TRUNCATE right when it
>    creates a new file.
> 
> truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> right.
> 
> ftruncate() (on a file): requires that the file had the TRUNCATE right
> when it was previously opened.
> 
> Signed-off-by: Günther Noack <[e-mail]>
> ---
>   include/uapi/linux/landlock.h                | 18 ++--
>   security/landlock/fs.c                       | 88 +++++++++++++++++++-
>   security/landlock/fs.h                       | 18 ++++
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/setup.c                    |  1 +
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   |  7 +-
>   8 files changed, 124 insertions(+), 14 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 23df4e0e8ace..8c0124c5cbe6 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -95,8 +95,16 @@ struct landlock_path_beneath_attr {
>    * A file can only receive these access rights:
>    *
>    * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
> - * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
> + * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
> + *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
> + *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
> + *   :manpage:`creat(2)`.
>    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
> + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
> + *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
> + *   `O_TRUNC`. The right to truncate a file gets carried along with an opened
> + *   file descriptor for the purpose of :manpage:`ftruncate(2)`.

You can add a bit to explain that it is the same behavior as for 
LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE .


> This access
> + *   right is available since the third version of the Landlock ABI.
>    *
>    * A directory can receive access rights related to files or directories.  The
>    * following access right is applied to the directory itself, and the
> @@ -139,10 +147,9 @@ struct landlock_path_beneath_attr {
>    *
>    *   It is currently not possible to restrict some file-related actions
>    *   accessible through these syscall families: :manpage:`chdir(2)`,
> - *   :manpage:`truncate(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:`chmod(2)`,
> + *   :manpage:`chown(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 */
> @@ -160,6 +167,7 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
> +#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>   /* clang-format on */
>   
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index a9dbd99d9ee7..1b546edf69a6 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   /* clang-format on */
>   
>   /*
> @@ -761,6 +762,47 @@ static bool collect_domain_accesses(
>   	return ret;
>   }
>   
> +/**
> + * get_path_access_rights - Returns the subset of rights in access_request
> + * which are permitted for the given path.
> + *
> + * @domain: The domain that defines the current restrictions.
> + * @path: The path to get access rights for.
> + * @access_request: The rights we are interested in.
> + *
> + * Returns: The access mask of the rights that are permitted on the given path,
> + * which are also a subset of access_request (to save some calculation time).
> + */
> +static inline access_mask_t
> +get_path_access_rights(const struct landlock_ruleset *const domain,
> +		       const struct path *const path,
> +		       access_mask_t access_request)
> +{
> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> +	unsigned long access_bit;
> +	unsigned long access_req;

unsigned long access_bit, long access_req;


> +
> +	init_layer_masks(domain, access_request, &layer_masks);
> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> +				    NULL, 0, NULL, NULL)) {
> +		/*
> +		 * Return immediately for successful accesses and for cases

Returns


> +		 * where everything is permitted because the path belongs to an
> +		 * internal filesystem.
> +		 */
> +		return access_request;
> +	}
> +
> +	access_req = access_request;
> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> +		if (layer_masks[access_bit]) {
> +			/* If any layer vetoed the access right, remove it. */
> +			access_request &= ~BIT_ULL(access_bit);
> +		}
> +	}
> +	return access_request;
> +}
> +
>   /**
>    * current_check_refer_path - Check if a rename or link action is allowed
>    *
> @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
>   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>   }
>   
> +static int hook_path_truncate(const struct path *const path)
> +{
> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> +}
> +
>   /* File hooks */
>   
>   static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
>   	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
>   	if (file->f_flags & __FMODE_EXEC)
>   		access |= LANDLOCK_ACCESS_FS_EXECUTE;
> +

Not needed.


>   	return access;
>   }
>   
>   static int hook_file_open(struct file *const file)
>   {
> +	access_mask_t access_req, access_rights;
> +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
>   	const struct landlock_ruleset *const dom =
>   		landlock_get_current_domain();
>   
> -	if (!dom)
> +	if (!dom) {
> +		/* Grant all rights. */

Something like:
Grants all rights, even if most of them are not checked here, it is more 
consistent.


> +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
>   		return 0;
> +	}
> +
>   	/*
>   	 * Because a file may be opened with O_PATH, get_file_access() may
>   	 * return 0.  This case will be handled with a future Landlock
>   	 * evolution.
>   	 */
> -	return check_access_path(dom, &file->f_path, get_file_access(file));
> +	access_req = get_file_access(file);
> +	access_rights = get_path_access_rights(dom, &file->f_path,
> +					       access_req | optional_rights);
> +	if (access_req & ~access_rights)
> +		return -EACCES;
> +
> +	/*
> +	 * For operations on already opened files (i.e. ftruncate()), it is the
> +	 * access rights at the time of open() which decide whether the
> +	 * operation is permitted. Therefore, we record the relevant subset of
> +	 * file access rights in the opened struct file.
> +	 */
> +	landlock_file(file)->rights = access_rights;
> +

Style preferences, but why do you use a new line here? I try to group 
code blocks until the return.


> +	return 0;
> +}
> +
> +static int hook_file_truncate(struct file *const file)
> +{
> +	/*
> +	 * We permit truncation if the truncation right was available at the

Allows truncation if the related right was…


> +	 * time of opening the file.

…to get a consistent access check as for read, write and execute operations.

This kind of explanation could be used to complete the documentation as 
well. The idea being to mimic the file mode check.


> +	 */
> +	if (!(landlock_file(file)->rights & LANDLOCK_ACCESS_FS_TRUNCATE))

I prefer to invert the "if" logic and return -EACCES by default.


> +		return -EACCES;
> +
> +	return 0;
>   }
>   
>   static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> @@ -1194,6 +1274,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
>   	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(file_truncate, hook_file_truncate),
>   
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   };
> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index 8db7acf9109b..275ba5375839 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -36,6 +36,18 @@ struct landlock_inode_security {
>   	struct landlock_object __rcu *object;
>   };
>   
> +/**
> + * struct landlock_file_security - File security blob
> + *
> + * This information is populated when opening a file in hook_file_open, and
> + * tracks the relevant Landlock access rights that were available at the time
> + * of opening the file. Other LSM hooks use these rights in order to authorize
> + * operations on already opened files.
> + */
> +struct landlock_file_security {
> +	access_mask_t rights;

I think it would make it more consistent to name it "access" to be in 
line with struct landlock_layer and other types.


> +};
> +
>   /**
>    * struct landlock_superblock_security - Superblock security blob
>    *
> @@ -50,6 +62,12 @@ struct landlock_superblock_security {
>   	atomic_long_t inode_refs;
>   };
>   
> +static inline struct landlock_file_security *
> +landlock_file(const struct file *const file)
> +{
> +	return file->f_security + landlock_blob_sizes.lbs_file;
> +}
> +
>   static inline struct landlock_inode_security *
>   landlock_inode(const struct inode *const inode)
>   {
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index b54184ab9439..82288f0e9e5e 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_REFER
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
>   #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/setup.c b/security/landlock/setup.c
> index f8e8e980454c..3f196d2ce4f9 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -19,6 +19,7 @@ bool landlock_initialized __lsm_ro_after_init = false;
>   
>   struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
>   	.lbs_cred = sizeof(struct landlock_cred_security),
> +	.lbs_file = sizeof(struct landlock_file_security),
>   	.lbs_inode = sizeof(struct landlock_inode_security),
>   	.lbs_superblock = sizeof(struct landlock_superblock_security),
>   };
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 735a0865ea11..f4d6fc7ed17f 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 2
> +#define LANDLOCK_ABI_VERSION 3
>   
>   /**
>    * 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 da9290817866..72cdae277b02 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(2, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(3, 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 45de42a027c5..87b28d14a1aa 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -406,9 +406,10 @@ TEST_F_FORK(layout1, inval)
>   #define ACCESS_FILE ( \
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> -	LANDLOCK_ACCESS_FS_READ_FILE)
> +	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_TRUNCATE)
>   
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
>   
>   #define ACCESS_ALL ( \
>   	ACCESS_FILE | \
> @@ -422,7 +423,7 @@ TEST_F_FORK(layout1, inval)
>   	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
>   	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
>   	LANDLOCK_ACCESS_FS_MAKE_SYM | \
> -	ACCESS_LAST)
> +	LANDLOCK_ACCESS_FS_REFER)
>   
>   /* clang-format on */
>

Günther Noack Sept. 12, 2022, 3:28 p.m. UTC | #2

On Fri, Sep 09, 2022 at 03:51:16PM +0200, Mickaël Salaün wrote:
> 
> On 08/09/2022 21:58, Günther Noack wrote:
> > Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> > 
> > This flag hooks into the path_truncate LSM hook and covers file
> > truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
> > well as creat().
> > 
> > This change also increments the Landlock ABI version, updates
> > corresponding selftests, and updates code documentation to document
> > the flag.
> > 
> > The following operations are restricted:
> > 
> > open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
> > implicitly truncated as part of the open() (e.g. using O_TRUNC).
> > 
> > Notable special cases:
> > * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
> > * open() with O_TRUNC does *not* need the TRUNCATE right when it
> >    creates a new file.
> > 
> > truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
> > right.
> > 
> > ftruncate() (on a file): requires that the file had the TRUNCATE right
> > when it was previously opened.
> > 
> > Signed-off-by: Günther Noack <[e-mail]>
> > ---
> >   include/uapi/linux/landlock.h                | 18 ++--
> >   security/landlock/fs.c                       | 88 +++++++++++++++++++-
> >   security/landlock/fs.h                       | 18 ++++
> >   security/landlock/limits.h                   |  2 +-
> >   security/landlock/setup.c                    |  1 +
> >   security/landlock/syscalls.c                 |  2 +-
> >   tools/testing/selftests/landlock/base_test.c |  2 +-
> >   tools/testing/selftests/landlock/fs_test.c   |  7 +-
> >   8 files changed, 124 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 23df4e0e8ace..8c0124c5cbe6 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
> > + *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
> > + *   `O_TRUNC`. The right to truncate a file gets carried along with an opened
> > + *   file descriptor for the purpose of :manpage:`ftruncate(2)`.
> 
> You can add a bit to explain that it is the same behavior as for
> LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE .

Done.

> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index a9dbd99d9ee7..1b546edf69a6 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > +static inline access_mask_t
> > +get_path_access_rights(const struct landlock_ruleset *const domain,
> > +		       const struct path *const path,
> > +		       access_mask_t access_request)
> > +{
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > +	unsigned long access_bit;
> > +	unsigned long access_req;
> 
> unsigned long access_bit, long access_req;

Done. Made it unsigned long access_bit, access_req;

> > +	init_layer_masks(domain, access_request, &layer_masks);
> > +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> > +				    NULL, 0, NULL, NULL)) {
> > +		/*
> > +		 * Return immediately for successful accesses and for cases
> 
> Returns

Done.

> > +		 * where everything is permitted because the path belongs to an
> > +		 * internal filesystem.
> > +		 */
> > +		return access_request;
> > +	}
> > +
> > +	access_req = access_request;
> > +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> > +		if (layer_masks[access_bit]) {
> > +			/* If any layer vetoed the access right, remove it. */
> > +			access_request &= ~BIT_ULL(access_bit);
> > +		}
> > +	}
> > +	return access_request;
> > +}
> > +
> >   /**
> >    * current_check_refer_path - Check if a rename or link action is allowed
> >    *
> > @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
> >   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> >   }
> > +static int hook_path_truncate(const struct path *const path)
> > +{
> > +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> > +}
> > +
> >   /* File hooks */
> >   static inline access_mask_t get_file_access(const struct file *const file)
> > @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
> >   	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
> >   	if (file->f_flags & __FMODE_EXEC)
> >   		access |= LANDLOCK_ACCESS_FS_EXECUTE;
> > +
> 
> Not needed.

Done.

> >   	return access;
> >   }
> >   static int hook_file_open(struct file *const file)
> >   {
> > +	access_mask_t access_req, access_rights;
> > +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
> >   	const struct landlock_ruleset *const dom =
> >   		landlock_get_current_domain();
> > -	if (!dom)
> > +	if (!dom) {
> > +		/* Grant all rights. */
> 
> Something like:
> Grants all rights, even if most of them are not checked here, it is more
> consistent.

Done.

> > +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
> >   		return 0;
> > +	}
> > +
> >   	/*
> >   	 * Because a file may be opened with O_PATH, get_file_access() may
> >   	 * return 0.  This case will be handled with a future Landlock
> >   	 * evolution.
> >   	 */
> > -	return check_access_path(dom, &file->f_path, get_file_access(file));
> > +	access_req = get_file_access(file);
> > +	access_rights = get_path_access_rights(dom, &file->f_path,
> > +					       access_req | optional_rights);
> > +	if (access_req & ~access_rights)
> > +		return -EACCES;
> > +
> > +	/*
> > +	 * For operations on already opened files (i.e. ftruncate()), it is the
> > +	 * access rights at the time of open() which decide whether the
> > +	 * operation is permitted. Therefore, we record the relevant subset of
> > +	 * file access rights in the opened struct file.
> > +	 */
> > +	landlock_file(file)->rights = access_rights;
> > +
> 
> Style preferences, but why do you use a new line here? I try to group code
> blocks until the return.

Thanks, done. I just do this habitually and overlooked that I was at
odds with the surrounding style. I don't have a strong preference.

> > +	return 0;
> > +}
> > +
> > +static int hook_file_truncate(struct file *const file)
> > +{
> > +	/*
> > +	 * We permit truncation if the truncation right was available at the
> 
> Allows truncation if the related right was…
> 
> 
> > +	 * time of opening the file.
> 
> …to get a consistent access check as for read, write and execute operations.

Done.

I'm also adding this note here:

  Note: For checks done based on the file's Landlock rights, we enforce
  them independently of whether the current thread is in a Landlock
  domain, so that open files passed between independent processes
  retain their behaviour.

to explain that this is why we don't check for "if (!dom)" as we do in
other cases.


> This kind of explanation could be used to complete the documentation as
> well. The idea being to mimic the file mode check.

Added it to the documentation.

> 
> 
> > +	 */
> > +	if (!(landlock_file(file)->rights & LANDLOCK_ACCESS_FS_TRUNCATE))
> 
> I prefer to invert the "if" logic and return -EACCES by default.

Done. Thanks for pointing it out.

> > +		return -EACCES;
> > +
> > +	return 0;
> >   }


> > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > index 8db7acf9109b..275ba5375839 100644
> > --- a/security/landlock/fs.h
> > +++ b/security/landlock/fs.h
> > @@ -36,6 +36,18 @@ struct landlock_inode_security {
> >   	struct landlock_object __rcu *object;
> >   };
> > +/**
> > + * struct landlock_file_security - File security blob
> > + *
> > + * This information is populated when opening a file in hook_file_open, and
> > + * tracks the relevant Landlock access rights that were available at the time
> > + * of opening the file. Other LSM hooks use these rights in order to authorize
> > + * operations on already opened files.
> > + */
> > +struct landlock_file_security {
> > +	access_mask_t rights;
> 
> I think it would make it more consistent to name it "access" to be in line
> with struct landlock_layer and other types.

Done.

I also added a brief documentation for the access field, to point out
that this is not the *full* set of rights which was available at
open() time, but it's just the subset of rights that is needed to
authorize later operations on the file:

  @access: Access rights that were available at the time of opening the
  file. This is not necessarily the full set of access rights available
  at that time, but it's the necessary subset as needed to authorize
  later operations on the open file.

Thanks for the review! Fixes will be in the next version.

-Günther

--

Mickaël Salaün Sept. 12, 2022, 6:37 p.m. UTC | #3

On 12/09/2022 17:28, Günther Noack wrote:
> On Fri, Sep 09, 2022 at 03:51:16PM +0200, Mickaël Salaün wrote:
>>
>> On 08/09/2022 21:58, Günther Noack wrote:
>>> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
>>>
>>> This flag hooks into the path_truncate LSM hook and covers file
>>> truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
>>> well as creat().
>>>
>>> This change also increments the Landlock ABI version, updates
>>> corresponding selftests, and updates code documentation to document
>>> the flag.
>>>
>>> The following operations are restricted:
>>>
>>> open(): requires the LANDLOCK_ACCESS_FS_TRUNCATE right if a file gets
>>> implicitly truncated as part of the open() (e.g. using O_TRUNC).
>>>
>>> Notable special cases:
>>> * open(..., O_RDONLY|O_TRUNC) can truncate files as well in Linux
>>> * open() with O_TRUNC does *not* need the TRUNCATE right when it
>>>     creates a new file.
>>>
>>> truncate() (on a path): requires the LANDLOCK_ACCESS_FS_TRUNCATE
>>> right.
>>>
>>> ftruncate() (on a file): requires that the file had the TRUNCATE right
>>> when it was previously opened.
>>>
>>> Signed-off-by: Günther Noack <[e-mail]>
>>> ---
>>>    include/uapi/linux/landlock.h                | 18 ++--
>>>    security/landlock/fs.c                       | 88 +++++++++++++++++++-
>>>    security/landlock/fs.h                       | 18 ++++
>>>    security/landlock/limits.h                   |  2 +-
>>>    security/landlock/setup.c                    |  1 +
>>>    security/landlock/syscalls.c                 |  2 +-
>>>    tools/testing/selftests/landlock/base_test.c |  2 +-
>>>    tools/testing/selftests/landlock/fs_test.c   |  7 +-
>>>    8 files changed, 124 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>>> index 23df4e0e8ace..8c0124c5cbe6 100644
>>> --- a/include/uapi/linux/landlock.h
>>> +++ b/include/uapi/linux/landlock.h
>>> + * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
>>> + *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
>>> + *   `O_TRUNC`. The right to truncate a file gets carried along with an opened
>>> + *   file descriptor for the purpose of :manpage:`ftruncate(2)`.
>>
>> You can add a bit to explain that it is the same behavior as for
>> LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE .
> 
> Done.
> 
>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>> index a9dbd99d9ee7..1b546edf69a6 100644
>>> --- a/security/landlock/fs.c
>>> +++ b/security/landlock/fs.c
>>> +static inline access_mask_t
>>> +get_path_access_rights(const struct landlock_ruleset *const domain,
>>> +		       const struct path *const path,
>>> +		       access_mask_t access_request)
>>> +{
>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>> +	unsigned long access_bit;
>>> +	unsigned long access_req;
>>
>> unsigned long access_bit, long access_req;
> 
> Done. Made it unsigned long access_bit, access_req;
> 
>>> +	init_layer_masks(domain, access_request, &layer_masks);
>>> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
>>> +				    NULL, 0, NULL, NULL)) {
>>> +		/*
>>> +		 * Return immediately for successful accesses and for cases
>>
>> Returns
> 
> Done.
> 
>>> +		 * where everything is permitted because the path belongs to an
>>> +		 * internal filesystem.
>>> +		 */
>>> +		return access_request;
>>> +	}
>>> +
>>> +	access_req = access_request;
>>> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
>>> +		if (layer_masks[access_bit]) {
>>> +			/* If any layer vetoed the access right, remove it. */
>>> +			access_request &= ~BIT_ULL(access_bit);
>>> +		}
>>> +	}
>>> +	return access_request;
>>> +}
>>> +
>>>    /**
>>>     * current_check_refer_path - Check if a rename or link action is allowed
>>>     *
>>> @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
>>>    	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>>>    }
>>> +static int hook_path_truncate(const struct path *const path)
>>> +{
>>> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>>> +}
>>> +
>>>    /* File hooks */
>>>    static inline access_mask_t get_file_access(const struct file *const file)
>>> @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
>>>    	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
>>>    	if (file->f_flags & __FMODE_EXEC)
>>>    		access |= LANDLOCK_ACCESS_FS_EXECUTE;
>>> +
>>
>> Not needed.
> 
> Done.
> 
>>>    	return access;
>>>    }
>>>    static int hook_file_open(struct file *const file)
>>>    {
>>> +	access_mask_t access_req, access_rights;
>>> +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
>>>    	const struct landlock_ruleset *const dom =
>>>    		landlock_get_current_domain();
>>> -	if (!dom)
>>> +	if (!dom) {
>>> +		/* Grant all rights. */
>>
>> Something like:
>> Grants all rights, even if most of them are not checked here, it is more
>> consistent.
> 
> Done.
> 
>>> +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
>>>    		return 0;
>>> +	}
>>> +
>>>    	/*
>>>    	 * Because a file may be opened with O_PATH, get_file_access() may
>>>    	 * return 0.  This case will be handled with a future Landlock
>>>    	 * evolution.
>>>    	 */
>>> -	return check_access_path(dom, &file->f_path, get_file_access(file));
>>> +	access_req = get_file_access(file);
>>> +	access_rights = get_path_access_rights(dom, &file->f_path,
>>> +					       access_req | optional_rights);
>>> +	if (access_req & ~access_rights)
>>> +		return -EACCES;
>>> +
>>> +	/*
>>> +	 * For operations on already opened files (i.e. ftruncate()), it is the
>>> +	 * access rights at the time of open() which decide whether the
>>> +	 * operation is permitted. Therefore, we record the relevant subset of
>>> +	 * file access rights in the opened struct file.
>>> +	 */
>>> +	landlock_file(file)->rights = access_rights;
>>> +
>>
>> Style preferences, but why do you use a new line here? I try to group code
>> blocks until the return.
> 
> Thanks, done. I just do this habitually and overlooked that I was at
> odds with the surrounding style. I don't have a strong preference.
> 
>>> +	return 0;
>>> +}
>>> +
>>> +static int hook_file_truncate(struct file *const file)
>>> +{
>>> +	/*
>>> +	 * We permit truncation if the truncation right was available at the
>>
>> Allows truncation if the related right was…
>>
>>
>>> +	 * time of opening the file.
>>
>> …to get a consistent access check as for read, write and execute operations.
> 
> Done.
> 
> I'm also adding this note here:
> 
>    Note: For checks done based on the file's Landlock rights, we enforce
>    them independently of whether the current thread is in a Landlock
>    domain, so that open files passed between independent processes
>    retain their behaviour.
> 
> to explain that this is why we don't check for "if (!dom)" as we do in
> other cases.
> 
> 
>> This kind of explanation could be used to complete the documentation as
>> well. The idea being to mimic the file mode check.
> 
> Added it to the documentation.
> 
>>
>>
>>> +	 */
>>> +	if (!(landlock_file(file)->rights & LANDLOCK_ACCESS_FS_TRUNCATE))
>>
>> I prefer to invert the "if" logic and return -EACCES by default.
> 
> Done. Thanks for pointing it out.
> 
>>> +		return -EACCES;
>>> +
>>> +	return 0;
>>>    }
> 
> 
>>> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
>>> index 8db7acf9109b..275ba5375839 100644
>>> --- a/security/landlock/fs.h
>>> +++ b/security/landlock/fs.h
>>> @@ -36,6 +36,18 @@ struct landlock_inode_security {
>>>    	struct landlock_object __rcu *object;
>>>    };
>>> +/**
>>> + * struct landlock_file_security - File security blob
>>> + *
>>> + * This information is populated when opening a file in hook_file_open, and
>>> + * tracks the relevant Landlock access rights that were available at the time
>>> + * of opening the file. Other LSM hooks use these rights in order to authorize
>>> + * operations on already opened files.
>>> + */
>>> +struct landlock_file_security {
>>> +	access_mask_t rights;
>>
>> I think it would make it more consistent to name it "access" to be in line
>> with struct landlock_layer and other types.
> 
> Done.

Hmm, actually, "allowed_access" is more explicit. We could use other 
access-related fields for other purposes (e.g. cache).


> 
> I also added a brief documentation for the access field, to point out
> that this is not the *full* set of rights which was available at
> open() time, but it's just the subset of rights that is needed to
> authorize later operations on the file:
> 
>    @access: Access rights that were available at the time of opening the
>    file. This is not necessarily the full set of access rights available
>    at that time, but it's the necessary subset as needed to authorize
>    later operations on the open file.

Good!

> 
> Thanks for the review! Fixes will be in the next version.
> 
> -Günther
>

Günther Noack Sept. 12, 2022, 7:04 p.m. UTC | #4

On Mon, Sep 12, 2022 at 08:37:09PM +0200, Mickaël Salaün wrote:
> 
> 
> On 12/09/2022 17:28, Günther Noack wrote:
> > On Fri, Sep 09, 2022 at 03:51:16PM +0200, Mickaël Salaün wrote:
> > > 
> > > On 08/09/2022 21:58, Günther Noack wrote:
> > > > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > > > index 8db7acf9109b..275ba5375839 100644
> > > > --- a/security/landlock/fs.h
> > > > +++ b/security/landlock/fs.h
> > > > +/**
> > > > + * struct landlock_file_security - File security blob
> > > > + *
> > > > + * This information is populated when opening a file in hook_file_open, and
> > > > + * tracks the relevant Landlock access rights that were available at the time
> > > > + * of opening the file. Other LSM hooks use these rights in order to authorize
> > > > + * operations on already opened files.
> > > > + */
 > > > > +struct landlock_file_security {
> > > > +	access_mask_t rights;
> > > 
> > > I think it would make it more consistent to name it "access" to be in line
> > > with struct landlock_layer and other types.
> > 
> > Done.
> 
> Hmm, actually, "allowed_access" is more explicit. We could use other
> access-related fields for other purposes (e.g. cache).

Makes sense, renamed to allowed_access.

—Günther

--

Mickaël Salaün Sept. 12, 2022, 7:41 p.m. UTC | #5

On 08/09/2022 21:58, Günther Noack wrote:
> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.

[...]

> @@ -761,6 +762,47 @@ static bool collect_domain_accesses(
>   	return ret;
>   }
>   
> +/**
> + * get_path_access_rights - Returns the subset of rights in access_request
> + * which are permitted for the given path.
> + *
> + * @domain: The domain that defines the current restrictions.
> + * @path: The path to get access rights for.
> + * @access_request: The rights we are interested in.
> + *
> + * Returns: The access mask of the rights that are permitted on the given path,
> + * which are also a subset of access_request (to save some calculation time).
> + */
> +static inline access_mask_t
> +get_path_access_rights(const struct landlock_ruleset *const domain,
> +		       const struct path *const path,
> +		       access_mask_t access_request)
> +{
> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> +	unsigned long access_bit;
> +	unsigned long access_req;
> +
> +	init_layer_masks(domain, access_request, &layer_masks);
> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> +				    NULL, 0, NULL, NULL)) {
> +		/*
> +		 * Return immediately for successful accesses and for cases
> +		 * where everything is permitted because the path belongs to an
> +		 * internal filesystem.
> +		 */
> +		return access_request;
> +	}
> +
> +	access_req = access_request;
> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> +		if (layer_masks[access_bit]) {
> +			/* If any layer vetoed the access right, remove it. */
> +			access_request &= ~BIT_ULL(access_bit);
> +		}
> +	}

This seems to be redundant with the value returned by 
init_layer_masks(), which should be passed to check_access_path_dual() 
to avoid useless path walk.

This function is pretty similar to check_access_path(). Can't you change 
it to use an access_mask_t pointer and get almost the same thing?


> +	return access_request;
> +}
> +
>   /**
>    * current_check_refer_path - Check if a rename or link action is allowed
>    *
> @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
>   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>   }
>   
> +static int hook_path_truncate(const struct path *const path)
> +{
> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> +}
> +
>   /* File hooks */
>   
>   static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
>   	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
>   	if (file->f_flags & __FMODE_EXEC)
>   		access |= LANDLOCK_ACCESS_FS_EXECUTE;
> +
>   	return access;
>   }
>   
>   static int hook_file_open(struct file *const file)
>   {
> +	access_mask_t access_req, access_rights;

"access_request" is used for access_mask_t, and "access_req" for 
unsigned int. I'd like to stick to this convention.


> +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;

You use "rights" often and I'm having some trouble to find a rational 
for that (compared to "access")…


>   	const struct landlock_ruleset *const dom =
>   		landlock_get_current_domain();
>   
> -	if (!dom)
> +	if (!dom) {
> +		/* Grant all rights. */
> +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
>   		return 0;
> +	}
> +
>   	/*
>   	 * Because a file may be opened with O_PATH, get_file_access() may
>   	 * return 0.  This case will be handled with a future Landlock
>   	 * evolution.
>   	 */
> -	return check_access_path(dom, &file->f_path, get_file_access(file));
> +	access_req = get_file_access(file);
> +	access_rights = get_path_access_rights(dom, &file->f_path,
> +					       access_req | optional_rights);
> +	if (access_req & ~access_rights)
> +		return -EACCES;

We should add a test to make sure this (optional_rights) logic is 
correct (and doesn't change), with a matrix of cases involving a ruleset 
handling either FS_WRITE, FS_TRUNCATE or both. This should be easy to do 
with test variants.


> +
> +	/*
> +	 * For operations on already opened files (i.e. ftruncate()), it is the
> +	 * access rights at the time of open() which decide whether the
> +	 * operation is permitted. Therefore, we record the relevant subset of
> +	 * file access rights in the opened struct file.
> +	 */
> +	landlock_file(file)->rights = access_rights;
> +
> +	return 0;
> +}

Günther Noack Sept. 23, 2022, 11:21 a.m. UTC | #6

On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
> 
> On 08/09/2022 21:58, Günther Noack wrote:
> > Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> 
> [...]
> 
> > @@ -761,6 +762,47 @@ static bool collect_domain_accesses(
> >   	return ret;
> >   }
> > +/**
> > + * get_path_access_rights - Returns the subset of rights in access_request
> > + * which are permitted for the given path.
> > + *
> > + * @domain: The domain that defines the current restrictions.
> > + * @path: The path to get access rights for.
> > + * @access_request: The rights we are interested in.
> > + *
> > + * Returns: The access mask of the rights that are permitted on the given path,
> > + * which are also a subset of access_request (to save some calculation time).
> > + */
> > +static inline access_mask_t
> > +get_path_access_rights(const struct landlock_ruleset *const domain,
> > +		       const struct path *const path,
> > +		       access_mask_t access_request)
> > +{
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > +	unsigned long access_bit;
> > +	unsigned long access_req;
> > +
> > +	init_layer_masks(domain, access_request, &layer_masks);
> > +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> > +				    NULL, 0, NULL, NULL)) {
> > +		/*
> > +		 * Return immediately for successful accesses and for cases
> > +		 * where everything is permitted because the path belongs to an
> > +		 * internal filesystem.
> > +		 */
> > +		return access_request;
> > +	}
> > +
> > +	access_req = access_request;
> > +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> > +		if (layer_masks[access_bit]) {
> > +			/* If any layer vetoed the access right, remove it. */
> > +			access_request &= ~BIT_ULL(access_bit);
> > +		}
> > +	}
> 
> This seems to be redundant with the value returned by init_layer_masks(),
> which should be passed to check_access_path_dual() to avoid useless path
> walk.

True, I'll use the result of init_layer_masks() to feed it back to
check_access_path_dual() to avoid a bit of computation.

Like this:

        effective_access_request =
		init_layer_masks(domain, access_request, &layer_masks);
	if (!check_access_path_dual(domain, path, effective_access_request,
	    &layer_masks, NULL, 0, NULL, NULL)) {
		// ...
	}

Overall, the approach here is:

* Initialize the layer_masks, so that it has a bit set for every
  access right in access_request and layer where that access right is
  handled.

* check_access_path_dual() with only the first few parameters -- this
  will clear all the bits in layer masks which are actually permitted
  according to the individual rules.

  As a special case, this *may* return 0 immediately, in which case we
  can (a) save a bit of calculation in the loop below and (b) we might
  be in the case where access is permitted because it's a file from a
  special file system (even though not all bits are cleared). If
  check_access_path_dual() returns 0, we return the full requested
  access_request that we received as input.

* In the loop below, if there are any bits left in layer_masks, those
  are rights which are not permitted for the given path. We remove
  these from access_request and return the modified access_request.


> This function is pretty similar to check_access_path(). Can't you change it
> to use an access_mask_t pointer and get almost the same thing?

I'm shying away from this approach. Many of the existing different use
cases are already realized by "doing if checks deep down". I think it
would make the code more understandable if we managed to model these
differences between use cases already at the layer of function calls.
(This is particularly true for check_access_path_dual(), where in
order to find out how the "single" case works, you need to disentangle
to a large extent how the much more complicated dual case works.)

If you want to unify these two functions, what do you think of the
approach of just using get_path_access_rights() instead of
check_access_path()?

Basically, it would turn

return check_access_path(dom, path, access_request);

into

if (get_path_access_rights(dom, path, access_request) == access_request)
	return 0;
return -EACCES;

This is slightly more verbose in the places where it's called, but it
would be more orthogonal, and it would also clarify that -EACCES is
the only possible error in the "single" path walk case.

Let me know what you think.

> > +	return access_request;
> > +}
> > +
> >   /**
> >    * current_check_refer_path - Check if a rename or link action is allowed
> >    *
> > @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
> >   	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> >   }
> > +static int hook_path_truncate(const struct path *const path)
> > +{
> > +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> > +}
> > +
> >   /* File hooks */
> >   static inline access_mask_t get_file_access(const struct file *const file)
> > @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
> >   	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
> >   	if (file->f_flags & __FMODE_EXEC)
> >   		access |= LANDLOCK_ACCESS_FS_EXECUTE;
> > +
> >   	return access;
> >   }
> >   static int hook_file_open(struct file *const file)
> >   {
> > +	access_mask_t access_req, access_rights;
> 
> "access_request" is used for access_mask_t, and "access_req" for unsigned
> int. I'd like to stick to this convention.

Done.

> > +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
> 
> You use "rights" often and I'm having some trouble to find a rational for
> that (compared to "access")…

Done. Didn't realize you already had a different convention here.

I'm renaming get_path_access_rights() to get_path_access() as well
then (and I'll rename get_file_access() to
get_required_file_open_access() - that's more verbose, but it sounded
too similar to get_path_access(), and it might be better to clarify
that this is a helper for the file_open hook). Does that sound
reasonable?


> >   	const struct landlock_ruleset *const dom =
> >   		landlock_get_current_domain();
> > -	if (!dom)
> > +	if (!dom) {
> > +		/* Grant all rights. */
> > +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
> >   		return 0;
> > +	}
> > +
> >   	/*
> >   	 * Because a file may be opened with O_PATH, get_file_access() may
> >   	 * return 0.  This case will be handled with a future Landlock
> >   	 * evolution.
> >   	 */
> > -	return check_access_path(dom, &file->f_path, get_file_access(file));
> > +	access_req = get_file_access(file);
> > +	access_rights = get_path_access_rights(dom, &file->f_path,
> > +					       access_req | optional_rights);
> > +	if (access_req & ~access_rights)
> > +		return -EACCES;
> 
> We should add a test to make sure this (optional_rights) logic is correct
> (and doesn't change), with a matrix of cases involving a ruleset handling
> either FS_WRITE, FS_TRUNCATE or both. This should be easy to do with test
> variants.

OK, adding one to the selftests.

> > +	/*
> > +	 * For operations on already opened files (i.e. ftruncate()), it is the
> > +	 * access rights at the time of open() which decide whether the
> > +	 * operation is permitted. Therefore, we record the relevant subset of
> > +	 * file access rights in the opened struct file.
> > +	 */
> > +	landlock_file(file)->rights = access_rights;
> > +
> > +	return 0;
> > +}

--

Mickaël Salaün Sept. 23, 2022, 8:53 p.m. UTC | #7

On 23/09/2022 13:21, Günther Noack wrote:
> On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
>>
>> On 08/09/2022 21:58, Günther Noack wrote:
>>> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
>>
>> [...]
>>
>>> @@ -761,6 +762,47 @@ static bool collect_domain_accesses(
>>>    	return ret;
>>>    }
>>> +/**
>>> + * get_path_access_rights - Returns the subset of rights in access_request
>>> + * which are permitted for the given path.
>>> + *
>>> + * @domain: The domain that defines the current restrictions.
>>> + * @path: The path to get access rights for.
>>> + * @access_request: The rights we are interested in.
>>> + *
>>> + * Returns: The access mask of the rights that are permitted on the given path,
>>> + * which are also a subset of access_request (to save some calculation time).
>>> + */
>>> +static inline access_mask_t
>>> +get_path_access_rights(const struct landlock_ruleset *const domain,
>>> +		       const struct path *const path,
>>> +		       access_mask_t access_request)
>>> +{
>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>> +	unsigned long access_bit;
>>> +	unsigned long access_req;
>>> +
>>> +	init_layer_masks(domain, access_request, &layer_masks);
>>> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
>>> +				    NULL, 0, NULL, NULL)) {
>>> +		/*
>>> +		 * Return immediately for successful accesses and for cases
>>> +		 * where everything is permitted because the path belongs to an
>>> +		 * internal filesystem.
>>> +		 */
>>> +		return access_request;
>>> +	}
>>> +
>>> +	access_req = access_request;
>>> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
>>> +		if (layer_masks[access_bit]) {
>>> +			/* If any layer vetoed the access right, remove it. */
>>> +			access_request &= ~BIT_ULL(access_bit);
>>> +		}
>>> +	}
>>
>> This seems to be redundant with the value returned by init_layer_masks(),
>> which should be passed to check_access_path_dual() to avoid useless path
>> walk.
> 
> True, I'll use the result of init_layer_masks() to feed it back to
> check_access_path_dual() to avoid a bit of computation.
> 
> Like this:
> 
>          effective_access_request =
> 		init_layer_masks(domain, access_request, &layer_masks);
> 	if (!check_access_path_dual(domain, path, effective_access_request,
> 	    &layer_masks, NULL, 0, NULL, NULL)) {
> 		// ...
> 	}

correct

> 
> Overall, the approach here is:
> 
> * Initialize the layer_masks, so that it has a bit set for every
>    access right in access_request and layer where that access right is
>    handled.
> 
> * check_access_path_dual() with only the first few parameters -- this
>    will clear all the bits in layer masks which are actually permitted
>    according to the individual rules.
> 
>    As a special case, this *may* return 0 immediately, in which case we
>    can (a) save a bit of calculation in the loop below and (b) we might
>    be in the case where access is permitted because it's a file from a
>    special file system (even though not all bits are cleared). If
>    check_access_path_dual() returns 0, we return the full requested
>    access_request that we received as input. >
> * In the loop below, if there are any bits left in layer_masks, those
>    are rights which are not permitted for the given path. We remove
>    these from access_request and return the modified access_request.
> 
> 
>> This function is pretty similar to check_access_path(). Can't you change it
>> to use an access_mask_t pointer and get almost the same thing?
> 
> I'm shying away from this approach. Many of the existing different use
> cases are already realized by "doing if checks deep down". I think it
> would make the code more understandable if we managed to model these
> differences between use cases already at the layer of function calls.
> (This is particularly true for check_access_path_dual(), where in
> order to find out how the "single" case works, you need to disentangle
> to a large extent how the much more complicated dual case works.)

I agree that check_access_path_dual() is complex, but I couldn't find a 
better way.


> 
> If you want to unify these two functions, what do you think of the
> approach of just using get_path_access_rights() instead of
> check_access_path()?
> 
> Basically, it would turn
> 
> return check_access_path(dom, path, access_request);
> 
> into
> 
> if (get_path_access_rights(dom, path, access_request) == access_request)
> 	return 0;
> return -EACCES;
> 
> This is slightly more verbose in the places where it's called, but it
> would be more orthogonal, and it would also clarify that -EACCES is
> the only possible error in the "single" path walk case.
> 
> Let me know what you think.

What about adding an additional argument `access_mask_t *const 
access_allowed` to check_access_path_dual() which returns the set of 
accesses (i.e. access_masked_parent1 & access_masked_parent2) that could 
then be stored to landlock_file(file)->allowed_access? If this argument 
is NULL it should just be ignored. What is left from 
get_path_access_rights() could then be merged into hook_file_open().


> 
>>> +	return access_request;
>>> +}
>>> +
>>>    /**
>>>     * current_check_refer_path - Check if a rename or link action is allowed
>>>     *
>>> @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
>>>    	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
>>>    }
>>> +static int hook_path_truncate(const struct path *const path)
>>> +{
>>> +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>>> +}
>>> +
>>>    /* File hooks */
>>>    static inline access_mask_t get_file_access(const struct file *const file)
>>> @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
>>>    	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
>>>    	if (file->f_flags & __FMODE_EXEC)
>>>    		access |= LANDLOCK_ACCESS_FS_EXECUTE;
>>> +
>>>    	return access;
>>>    }
>>>    static int hook_file_open(struct file *const file)
>>>    {
>>> +	access_mask_t access_req, access_rights;
>>
>> "access_request" is used for access_mask_t, and "access_req" for unsigned
>> int. I'd like to stick to this convention.
> 
> Done.
> 
>>> +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
>>
>> You use "rights" often and I'm having some trouble to find a rational for
>> that (compared to "access")…
> 
> Done. Didn't realize you already had a different convention here.
> 
> I'm renaming get_path_access_rights() to get_path_access() as well
> then (and I'll rename get_file_access() to
> get_required_file_open_access() - that's more verbose, but it sounded
> too similar to get_path_access(), and it might be better to clarify
> that this is a helper for the file_open hook). Does that sound
> reasonable?

I think it is better, but I'm not convinced this helper is useful.

> 
> 
>>>    	const struct landlock_ruleset *const dom =
>>>    		landlock_get_current_domain();
>>> -	if (!dom)
>>> +	if (!dom) {
>>> +		/* Grant all rights. */
>>> +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
>>>    		return 0;
>>> +	}
>>> +
>>>    	/*
>>>    	 * Because a file may be opened with O_PATH, get_file_access() may
>>>    	 * return 0.  This case will be handled with a future Landlock
>>>    	 * evolution.
>>>    	 */
>>> -	return check_access_path(dom, &file->f_path, get_file_access(file));
>>> +	access_req = get_file_access(file);
>>> +	access_rights = get_path_access_rights(dom, &file->f_path,
>>> +					       access_req | optional_rights);
>>> +	if (access_req & ~access_rights)
>>> +		return -EACCES;
>>
>> We should add a test to make sure this (optional_rights) logic is correct
>> (and doesn't change), with a matrix of cases involving a ruleset handling
>> either FS_WRITE, FS_TRUNCATE or both. This should be easy to do with test
>> variants.
> 
> OK, adding one to the selftests.
> 
>>> +	/*
>>> +	 * For operations on already opened files (i.e. ftruncate()), it is the
>>> +	 * access rights at the time of open() which decide whether the
>>> +	 * operation is permitted. Therefore, we record the relevant subset of
>>> +	 * file access rights in the opened struct file.
>>> +	 */
>>> +	landlock_file(file)->rights = access_rights;
>>> +
>>> +	return 0;
>>> +}
>

Günther Noack Sept. 25, 2022, 6:09 p.m. UTC | #8

On Fri, Sep 23, 2022 at 10:53:23PM +0200, Mickaël Salaün wrote:
> On 23/09/2022 13:21, Günther Noack wrote:
> > On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
> > > On 08/09/2022 21:58, Günther Noack wrote:
> > > > Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> > > 
> > > [...]
> > > 
> > > > +/**
> > > > + * get_path_access_rights - Returns the subset of rights in access_request
> > > > + * which are permitted for the given path.
> > > > + *
> > > > + * @domain: The domain that defines the current restrictions.
> > > > + * @path: The path to get access rights for.
> > > > + * @access_request: The rights we are interested in.
> > > > + *
> > > > + * Returns: The access mask of the rights that are permitted on the given path,
> > > > + * which are also a subset of access_request (to save some calculation time).
> > > > + */
> > > > +static inline access_mask_t
> > > > +get_path_access_rights(const struct landlock_ruleset *const domain,
> > > > +		       const struct path *const path,
> > > > +		       access_mask_t access_request)
> > > > +{
> > > > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > > > +	unsigned long access_bit;
> > > > +	unsigned long access_req;
> > > > +
> > > > +	init_layer_masks(domain, access_request, &layer_masks);
> > > > +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> > > > +				    NULL, 0, NULL, NULL)) {
> > > > +		/*
> > > > +		 * Return immediately for successful accesses and for cases
> > > > +		 * where everything is permitted because the path belongs to an
> > > > +		 * internal filesystem.
> > > > +		 */
> > > > +		return access_request;
> > > > +	}
> > > > +
> > > > +	access_req = access_request;
> > > > +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> > > > +		if (layer_masks[access_bit]) {
> > > > +			/* If any layer vetoed the access right, remove it. */
> > > > +			access_request &= ~BIT_ULL(access_bit);
> > > > +		}
> > > > +	}
> > > 
> > > This seems to be redundant with the value returned by init_layer_masks(),
> > > which should be passed to check_access_path_dual() to avoid useless path
> > > walk.
> > 
> > True, I'll use the result of init_layer_masks() to feed it back to
> > check_access_path_dual() to avoid a bit of computation.
> > 
> > Like this:
> > 
> >          effective_access_request =
> > 		init_layer_masks(domain, access_request, &layer_masks);
> > 	if (!check_access_path_dual(domain, path, effective_access_request,
> > 	    &layer_masks, NULL, 0, NULL, NULL)) {
> > 		// ...
> > 	}
> 
> correct
> 
> > 
> > Overall, the approach here is:
> > 
> > * Initialize the layer_masks, so that it has a bit set for every
> >    access right in access_request and layer where that access right is
> >    handled.
> > 
> > * check_access_path_dual() with only the first few parameters -- this
> >    will clear all the bits in layer masks which are actually permitted
> >    according to the individual rules.
> > 
> >    As a special case, this *may* return 0 immediately, in which case we
> >    can (a) save a bit of calculation in the loop below and (b) we might
> >    be in the case where access is permitted because it's a file from a
> >    special file system (even though not all bits are cleared). If
> >    check_access_path_dual() returns 0, we return the full requested
> >    access_request that we received as input. >
> > * In the loop below, if there are any bits left in layer_masks, those
> >    are rights which are not permitted for the given path. We remove
> >    these from access_request and return the modified access_request.
> > 
> > 
> > > This function is pretty similar to check_access_path(). Can't you change it
> > > to use an access_mask_t pointer and get almost the same thing?
> > 
> > I'm shying away from this approach. Many of the existing different use
> > cases are already realized by "doing if checks deep down". I think it
> > would make the code more understandable if we managed to model these
> > differences between use cases already at the layer of function calls.
> > (This is particularly true for check_access_path_dual(), where in
> > order to find out how the "single" case works, you need to disentangle
> > to a large extent how the much more complicated dual case works.)
> 
> I agree that check_access_path_dual() is complex, but I couldn't find a
> better way.

It seems out of the scope of this patch set, but I sometimes find it
OK to just duplicate the code and have a set of tests to demonstrate
that the two variants do the same thing.

check_access_path_dual() is mostly complex because of performance
reasons, as far as I can tell, and it might be possible to check its
results against a parallel implementation of it which runs slower,
uses more memory, but is more obviously correct. (I have used one
myself to check against when developing the truncate patch set.)

> > If you want to unify these two functions, what do you think of the
> > approach of just using get_path_access_rights() instead of
> > check_access_path()?
> > 
> > Basically, it would turn
> > 
> > return check_access_path(dom, path, access_request);
> > 
> > into
> > 
> > if (get_path_access_rights(dom, path, access_request) == access_request)
> > 	return 0;
> > return -EACCES;
> > 
> > This is slightly more verbose in the places where it's called, but it
> > would be more orthogonal, and it would also clarify that -EACCES is
> > the only possible error in the "single" path walk case.
> > 
> > Let me know what you think.
> 
> What about adding an additional argument `access_mask_t *const
> access_allowed` to check_access_path_dual() which returns the set of
> accesses (i.e. access_masked_parent1 & access_masked_parent2) that could
> then be stored to landlock_file(file)->allowed_access? If this argument is
> NULL it should just be ignored. What is left from get_path_access_rights()
> could then be merged into hook_file_open().

IMHO, check_access_path_dual() does not seem like the right place to
add this. This functionality is not needed in any of the "dual path"
cases so far, and I'm not sure what it would mean. The necessary
information can also be easily derived from the resulting layer_masks,
which is already exposed in the check_access_path_dual() interface,
and I also believe that this approach is at least equally fast as
updating it on the fly when changing the layer_masks.

I could be convinced to add a `access_mask_t *const access_allowed`
argument to check_access_path() if you prefer that, but then again, in
that case the returned boolean can be reconstructed from the new
access_allowed variable, and we could as well make check_access_path()
return the access_allowed result instead of the boolean and let
callers check equality with what they expected...? (I admittedly don't
have a good setup to test the performance right now, but it looks like
a negligible difference to me?)

Here are the options we have discussed, in the order that I would
prefer them:

* to keep it as a separate function as it already is,
  slightly duplicating check_access_path(). (I think it's cleaner,
  because the code path for the rest of the hooks other than
  security_file_open() stays simpler.)

* to make check_access_path() return the access_allowed access mask
  and make callers check that it covers the access_request that they
  asked for (see example from my previous mail on this thread). (This
  is equivalent to discarding the existing check_access_path() and
  using the get_path_access() function instead.)

* to add a `access_mask_t *const access_allowed` argument to
  check_access_path(), which is calculated if it's non-NULL based on
  the layer_masks result. It would be used from the security_file_open
  hook.

* to add a `access_mask_t *const access_allowed` argument to
  check_access_path_dual(). This doesn't make much sense, IMHO,
  because an on-the-fly calculation of this result does not look like
  a performance benefit to me, and calculating it based on the two
  resulting layer_masks is already possible now. It's also not clear
  to me what it would mean to calculate an access_allowed on two paths
  at once, and what that would be used for.

Let me know which option you prefer. In the end, I don't feel that
strongly about it and I'm happy to do this either way.


> > > > +	return access_request;
> > > > +}
> > > > +
> > > >    /**
> > > >     * current_check_refer_path - Check if a rename or link action is allowed
> > > >     *
> > > > @@ -1142,6 +1184,11 @@ static int hook_path_rmdir(const struct path *const dir,
> > > >    	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> > > >    }
> > > > +static int hook_path_truncate(const struct path *const path)
> > > > +{
> > > > +	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> > > > +}
> > > > +
> > > >    /* File hooks */
> > > >    static inline access_mask_t get_file_access(const struct file *const file)
> > > > @@ -1159,22 +1206,55 @@ static inline access_mask_t get_file_access(const struct file *const file)
> > > >    	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
> > > >    	if (file->f_flags & __FMODE_EXEC)
> > > >    		access |= LANDLOCK_ACCESS_FS_EXECUTE;
> > > > +
> > > >    	return access;
> > > >    }
> > > >    static int hook_file_open(struct file *const file)
> > > >    {
> > > > +	access_mask_t access_req, access_rights;
> > > 
> > > "access_request" is used for access_mask_t, and "access_req" for unsigned
> > > int. I'd like to stick to this convention.
> > 
> > Done.
> > 
> > > > +	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
> > > 
> > > You use "rights" often and I'm having some trouble to find a rational for
> > > that (compared to "access")…
> > 
> > Done. Didn't realize you already had a different convention here.
> > 
> > I'm renaming get_path_access_rights() to get_path_access() as well
> > then (and I'll rename get_file_access() to
> > get_required_file_open_access() - that's more verbose, but it sounded
> > too similar to get_path_access(), and it might be better to clarify
> > that this is a helper for the file_open hook). Does that sound
> > reasonable?
> 
> I think it is better, but I'm not convinced this helper is useful.
> 
> > 
> > 
> > > >    	const struct landlock_ruleset *const dom =
> > > >    		landlock_get_current_domain();
> > > > -	if (!dom)
> > > > +	if (!dom) {
> > > > +		/* Grant all rights. */
> > > > +		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
> > > >    		return 0;
> > > > +	}
> > > > +
> > > >    	/*
> > > >    	 * Because a file may be opened with O_PATH, get_file_access() may
> > > >    	 * return 0.  This case will be handled with a future Landlock
> > > >    	 * evolution.
> > > >    	 */
> > > > -	return check_access_path(dom, &file->f_path, get_file_access(file));
> > > > +	access_req = get_file_access(file);
> > > > +	access_rights = get_path_access_rights(dom, &file->f_path,
> > > > +					       access_req | optional_rights);
> > > > +	if (access_req & ~access_rights)
> > > > +		return -EACCES;
> > > 
> > > We should add a test to make sure this (optional_rights) logic is correct
> > > (and doesn't change), with a matrix of cases involving a ruleset handling
> > > either FS_WRITE, FS_TRUNCATE or both. This should be easy to do with test
> > > variants.
> > 
> > OK, adding one to the selftests.
> > 
> > > > +	/*
> > > > +	 * For operations on already opened files (i.e. ftruncate()), it is the
> > > > +	 * access rights at the time of open() which decide whether the
> > > > +	 * operation is permitted. Therefore, we record the relevant subset of
> > > > +	 * file access rights in the opened struct file.
> > > > +	 */
> > > > +	landlock_file(file)->rights = access_rights;
> > > > +
> > > > +	return 0;
> > > > +}
> > 

--

Mickaël Salaün Sept. 28, 2022, 6:32 p.m. UTC | #9

On 25/09/2022 20:09, Günther Noack wrote:
> On Fri, Sep 23, 2022 at 10:53:23PM +0200, Mickaël Salaün wrote:
>> On 23/09/2022 13:21, Günther Noack wrote:
>>> On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
>>>> On 08/09/2022 21:58, Günther Noack wrote:
>>>>> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
>>>>
>>>> [...]
>>>>
>>>>> +/**
>>>>> + * get_path_access_rights - Returns the subset of rights in access_request
>>>>> + * which are permitted for the given path.
>>>>> + *
>>>>> + * @domain: The domain that defines the current restrictions.
>>>>> + * @path: The path to get access rights for.
>>>>> + * @access_request: The rights we are interested in.
>>>>> + *
>>>>> + * Returns: The access mask of the rights that are permitted on the given path,
>>>>> + * which are also a subset of access_request (to save some calculation time).
>>>>> + */
>>>>> +static inline access_mask_t
>>>>> +get_path_access_rights(const struct landlock_ruleset *const domain,
>>>>> +		       const struct path *const path,
>>>>> +		       access_mask_t access_request)
>>>>> +{
>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>>>> +	unsigned long access_bit;
>>>>> +	unsigned long access_req;
>>>>> +
>>>>> +	init_layer_masks(domain, access_request, &layer_masks);
>>>>> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
>>>>> +				    NULL, 0, NULL, NULL)) {
>>>>> +		/*
>>>>> +		 * Return immediately for successful accesses and for cases
>>>>> +		 * where everything is permitted because the path belongs to an
>>>>> +		 * internal filesystem.
>>>>> +		 */
>>>>> +		return access_request;
>>>>> +	}
>>>>> +
>>>>> +	access_req = access_request;
>>>>> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
>>>>> +		if (layer_masks[access_bit]) {
>>>>> +			/* If any layer vetoed the access right, remove it. */
>>>>> +			access_request &= ~BIT_ULL(access_bit);
>>>>> +		}
>>>>> +	}
>>>>
>>>> This seems to be redundant with the value returned by init_layer_masks(),
>>>> which should be passed to check_access_path_dual() to avoid useless path
>>>> walk.
>>>
>>> True, I'll use the result of init_layer_masks() to feed it back to
>>> check_access_path_dual() to avoid a bit of computation.
>>>
>>> Like this:
>>>
>>>           effective_access_request =
>>> 		init_layer_masks(domain, access_request, &layer_masks);
>>> 	if (!check_access_path_dual(domain, path, effective_access_request,
>>> 	    &layer_masks, NULL, 0, NULL, NULL)) {
>>> 		// ...
>>> 	}
>>
>> correct
>>
>>>
>>> Overall, the approach here is:
>>>
>>> * Initialize the layer_masks, so that it has a bit set for every
>>>     access right in access_request and layer where that access right is
>>>     handled.
>>>
>>> * check_access_path_dual() with only the first few parameters -- this
>>>     will clear all the bits in layer masks which are actually permitted
>>>     according to the individual rules.
>>>
>>>     As a special case, this *may* return 0 immediately, in which case we
>>>     can (a) save a bit of calculation in the loop below and (b) we might
>>>     be in the case where access is permitted because it's a file from a
>>>     special file system (even though not all bits are cleared). If
>>>     check_access_path_dual() returns 0, we return the full requested
>>>     access_request that we received as input. >
>>> * In the loop below, if there are any bits left in layer_masks, those
>>>     are rights which are not permitted for the given path. We remove
>>>     these from access_request and return the modified access_request.
>>>
>>>
>>>> This function is pretty similar to check_access_path(). Can't you change it
>>>> to use an access_mask_t pointer and get almost the same thing?
>>>
>>> I'm shying away from this approach. Many of the existing different use
>>> cases are already realized by "doing if checks deep down". I think it
>>> would make the code more understandable if we managed to model these
>>> differences between use cases already at the layer of function calls.
>>> (This is particularly true for check_access_path_dual(), where in
>>> order to find out how the "single" case works, you need to disentangle
>>> to a large extent how the much more complicated dual case works.)
>>
>> I agree that check_access_path_dual() is complex, but I couldn't find a
>> better way.
> 
> It seems out of the scope of this patch set, but I sometimes find it
> OK to just duplicate the code and have a set of tests to demonstrate
> that the two variants do the same thing.
> 
> check_access_path_dual() is mostly complex because of performance
> reasons, as far as I can tell, and it might be possible to check its
> results against a parallel implementation of it which runs slower,
> uses more memory, but is more obviously correct. (I have used one
> myself to check against when developing the truncate patch set.)
> 
>>> If you want to unify these two functions, what do you think of the
>>> approach of just using get_path_access_rights() instead of
>>> check_access_path()?
>>>
>>> Basically, it would turn
>>>
>>> return check_access_path(dom, path, access_request);
>>>
>>> into
>>>
>>> if (get_path_access_rights(dom, path, access_request) == access_request)
>>> 	return 0;
>>> return -EACCES;
>>>
>>> This is slightly more verbose in the places where it's called, but it
>>> would be more orthogonal, and it would also clarify that -EACCES is
>>> the only possible error in the "single" path walk case.
>>>
>>> Let me know what you think.
>>
>> What about adding an additional argument `access_mask_t *const
>> access_allowed` to check_access_path_dual() which returns the set of
>> accesses (i.e. access_masked_parent1 & access_masked_parent2) that could
>> then be stored to landlock_file(file)->allowed_access? If this argument is
>> NULL it should just be ignored. What is left from get_path_access_rights()
>> could then be merged into hook_file_open().
> 
> IMHO, check_access_path_dual() does not seem like the right place to
> add this. This functionality is not needed in any of the "dual path"
> cases so far, and I'm not sure what it would mean. The necessary
> information can also be easily derived from the resulting layer_masks,
> which is already exposed in the check_access_path_dual() interface,
> and I also believe that this approach is at least equally fast as
> updating it on the fly when changing the layer_masks.
> 
> I could be convinced to add a `access_mask_t *const access_allowed`
> argument to check_access_path() if you prefer that, but then again, in
> that case the returned boolean can be reconstructed from the new
> access_allowed variable, and we could as well make check_access_path()
> return the access_allowed result instead of the boolean and let
> callers check equality with what they expected...? (I admittedly don't
> have a good setup to test the performance right now, but it looks like
> a negligible difference to me?)

Good idea, let's try to make check_access_path_dual() returns the 
allowed accesses (according to the request) and rename it to 
get_access_path_dual(). unmask_layers() could be changed to return the 
still-denied accesses instead of a boolean, and we could use this values 
(for potential both parents) to return allowed_parent1 & allowed_parent2 
(with access_mask_t types). This would also simplify is_eaccess() and 
its calls could be moved to current_check_refer_path(). This would merge 
get_path_access_rights() into check_access_path_dual() and make the 
errno codes more explicit per hook or defined in check_access_path().


> 
> Here are the options we have discussed, in the order that I would
> prefer them:
> 
> * to keep it as a separate function as it already is,
>    slightly duplicating check_access_path(). (I think it's cleaner,
>    because the code path for the rest of the hooks other than
>    security_file_open() stays simpler.)
> 
> * to make check_access_path() return the access_allowed access mask
>    and make callers check that it covers the access_request that they
>    asked for (see example from my previous mail on this thread). (This
>    is equivalent to discarding the existing check_access_path() and
>    using the get_path_access() function instead.)
> 
> * to add a `access_mask_t *const access_allowed` argument to
>    check_access_path(), which is calculated if it's non-NULL based on
>    the layer_masks result. It would be used from the security_file_open
>    hook.
> 
> * to add a `access_mask_t *const access_allowed` argument to
>    check_access_path_dual(). This doesn't make much sense, IMHO,
>    because an on-the-fly calculation of this result does not look like
>    a performance benefit to me, and calculating it based on the two
>    resulting layer_masks is already possible now. It's also not clear
>    to me what it would mean to calculate an access_allowed on two paths
>    at once, and what that would be used for.
> 
> Let me know which option you prefer. In the end, I don't feel that
> strongly about it and I'm happy to do this either way.

Günther Noack Sept. 29, 2022, 7:22 p.m. UTC | #10

On Wed, Sep 28, 2022 at 08:32:02PM +0200, Mickaël Salaün wrote:
> On 25/09/2022 20:09, Günther Noack wrote:
> > On Fri, Sep 23, 2022 at 10:53:23PM +0200, Mickaël Salaün wrote:
> > > On 23/09/2022 13:21, Günther Noack wrote:
> > > > On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
> > > > > On 08/09/2022 21:58, Günther Noack wrote:
> > > > > > Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > +/**
> > > > > > + * get_path_access_rights - Returns the subset of rights in access_request
> > > > > > + * which are permitted for the given path.
> > > > > > + *
> > > > > > + * @domain: The domain that defines the current restrictions.
> > > > > > + * @path: The path to get access rights for.
> > > > > > + * @access_request: The rights we are interested in.
> > > > > > + *
> > > > > > + * Returns: The access mask of the rights that are permitted on the given path,
> > > > > > + * which are also a subset of access_request (to save some calculation time).
> > > > > > + */
> > > > > > +static inline access_mask_t
> > > > > > +get_path_access_rights(const struct landlock_ruleset *const domain,
> > > > > > +		       const struct path *const path,
> > > > > > +		       access_mask_t access_request)
> > > > > > +{
> > > > > > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > > > > > +	unsigned long access_bit;
> > > > > > +	unsigned long access_req;
> > > > > > +
> > > > > > +	init_layer_masks(domain, access_request, &layer_masks);
> > > > > > +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
> > > > > > +				    NULL, 0, NULL, NULL)) {
> > > > > > +		/*
> > > > > > +		 * Return immediately for successful accesses and for cases
> > > > > > +		 * where everything is permitted because the path belongs to an
> > > > > > +		 * internal filesystem.
> > > > > > +		 */
> > > > > > +		return access_request;
> > > > > > +	}
> > > > > > +
> > > > > > +	access_req = access_request;
> > > > > > +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
> > > > > > +		if (layer_masks[access_bit]) {
> > > > > > +			/* If any layer vetoed the access right, remove it. */
> > > > > > +			access_request &= ~BIT_ULL(access_bit);
> > > > > > +		}
> > > > > > +	}
> > > > > 
> > > > > This seems to be redundant with the value returned by init_layer_masks(),
> > > > > which should be passed to check_access_path_dual() to avoid useless path
> > > > > walk.
> > > > 
> > > > True, I'll use the result of init_layer_masks() to feed it back to
> > > > check_access_path_dual() to avoid a bit of computation.
> > > > 
> > > > Like this:
> > > > 
> > > >           effective_access_request =
> > > > 		init_layer_masks(domain, access_request, &layer_masks);
> > > > 	if (!check_access_path_dual(domain, path, effective_access_request,
> > > > 	    &layer_masks, NULL, 0, NULL, NULL)) {
> > > > 		// ...
> > > > 	}
> > > 
> > > correct
> > > 
> > > > 
> > > > Overall, the approach here is:
> > > > 
> > > > * Initialize the layer_masks, so that it has a bit set for every
> > > >     access right in access_request and layer where that access right is
> > > >     handled.
> > > > 
> > > > * check_access_path_dual() with only the first few parameters -- this
> > > >     will clear all the bits in layer masks which are actually permitted
> > > >     according to the individual rules.
> > > > 
> > > >     As a special case, this *may* return 0 immediately, in which case we
> > > >     can (a) save a bit of calculation in the loop below and (b) we might
> > > >     be in the case where access is permitted because it's a file from a
> > > >     special file system (even though not all bits are cleared). If
> > > >     check_access_path_dual() returns 0, we return the full requested
> > > >     access_request that we received as input. >
> > > > * In the loop below, if there are any bits left in layer_masks, those
> > > >     are rights which are not permitted for the given path. We remove
> > > >     these from access_request and return the modified access_request.
> > > > 
> > > > 
> > > > > This function is pretty similar to check_access_path(). Can't you change it
> > > > > to use an access_mask_t pointer and get almost the same thing?
> > > > 
> > > > I'm shying away from this approach. Many of the existing different use
> > > > cases are already realized by "doing if checks deep down". I think it
> > > > would make the code more understandable if we managed to model these
> > > > differences between use cases already at the layer of function calls.
> > > > (This is particularly true for check_access_path_dual(), where in
> > > > order to find out how the "single" case works, you need to disentangle
> > > > to a large extent how the much more complicated dual case works.)
> > > 
> > > I agree that check_access_path_dual() is complex, but I couldn't find a
> > > better way.
> > 
> > It seems out of the scope of this patch set, but I sometimes find it
> > OK to just duplicate the code and have a set of tests to demonstrate
> > that the two variants do the same thing.
> > 
> > check_access_path_dual() is mostly complex because of performance
> > reasons, as far as I can tell, and it might be possible to check its
> > results against a parallel implementation of it which runs slower,
> > uses more memory, but is more obviously correct. (I have used one
> > myself to check against when developing the truncate patch set.)
> > 
> > > > If you want to unify these two functions, what do you think of the
> > > > approach of just using get_path_access_rights() instead of
> > > > check_access_path()?
> > > > 
> > > > Basically, it would turn
> > > > 
> > > > return check_access_path(dom, path, access_request);
> > > > 
> > > > into
> > > > 
> > > > if (get_path_access_rights(dom, path, access_request) == access_request)
> > > > 	return 0;
> > > > return -EACCES;
> > > > 
> > > > This is slightly more verbose in the places where it's called, but it
> > > > would be more orthogonal, and it would also clarify that -EACCES is
> > > > the only possible error in the "single" path walk case.
> > > > 
> > > > Let me know what you think.
> > > 
> > > What about adding an additional argument `access_mask_t *const
> > > access_allowed` to check_access_path_dual() which returns the set of
> > > accesses (i.e. access_masked_parent1 & access_masked_parent2) that could
> > > then be stored to landlock_file(file)->allowed_access? If this argument is
> > > NULL it should just be ignored. What is left from get_path_access_rights()
> > > could then be merged into hook_file_open().
> > 
> > IMHO, check_access_path_dual() does not seem like the right place to
> > add this. This functionality is not needed in any of the "dual path"
> > cases so far, and I'm not sure what it would mean. The necessary
> > information can also be easily derived from the resulting layer_masks,
> > which is already exposed in the check_access_path_dual() interface,
> > and I also believe that this approach is at least equally fast as
> > updating it on the fly when changing the layer_masks.
> > 
> > I could be convinced to add a `access_mask_t *const access_allowed`
> > argument to check_access_path() if you prefer that, but then again, in
> > that case the returned boolean can be reconstructed from the new
> > access_allowed variable, and we could as well make check_access_path()
> > return the access_allowed result instead of the boolean and let
> > callers check equality with what they expected...? (I admittedly don't
> > have a good setup to test the performance right now, but it looks like
> > a negligible difference to me?)
> 
> Good idea, let's try to make check_access_path_dual() returns the allowed
> accesses (according to the request) and rename it to get_access_path_dual().
> unmask_layers() could be changed to return the still-denied accesses instead
> of a boolean, and we could use this values (for potential both parents) to
> return allowed_parent1 & allowed_parent2 (with access_mask_t types). This
> would also simplify is_eaccess() and its calls could be moved to
> current_check_refer_path(). This would merge get_path_access_rights() into
> check_access_path_dual() and make the errno codes more explicit per hook or
> defined in check_access_path().

Thanks for the review!

I'm afraid I don't understand this approach at the moment. I'm
probably still missing some insight about how the "refer" logic works
which would make this clearer.

With the proposed changes to check_access_path_dual(), it sounds like
we would have to change the logic of the "refer" implementation quite
a bit, which would expand the scope of the "truncate" patch set beyond
what it was originally meant to do. Is this check_access_path_dual()
refactoring something you'd insist on for the truncate patch set, or
would you be OK with doing that separately?

For the truncate patch set, what do you think of the lighter
refactoring options, which I had outlined in my previous mail? - see
the four bullet points quoted here:

> > Here are the options we have discussed, in the order that I would
> > prefer them:
> > 
> > * to keep it as a separate function as it already is,
> >    slightly duplicating check_access_path(). (I think it's cleaner,
> >    because the code path for the rest of the hooks other than
> >    security_file_open() stays simpler.)
> > 
> > * to make check_access_path() return the access_allowed access mask
> >    and make callers check that it covers the access_request that they
> >    asked for (see example from my previous mail on this thread). (This
> >    is equivalent to discarding the existing check_access_path() and
> >    using the get_path_access() function instead.)
> > 
> > * to add a `access_mask_t *const access_allowed` argument to
> >    check_access_path(), which is calculated if it's non-NULL based on
> >    the layer_masks result. It would be used from the security_file_open
> >    hook.
> > 
> > * to add a `access_mask_t *const access_allowed` argument to
> >    check_access_path_dual(). This doesn't make much sense, IMHO,
> >    because an on-the-fly calculation of this result does not look like
> >    a performance benefit to me, and calculating it based on the two
> >    resulting layer_masks is already possible now. It's also not clear
> >    to me what it would mean to calculate an access_allowed on two paths
> >    at once, and what that would be used for.
> > 
> > Let me know which option you prefer. In the end, I don't feel that
> > strongly about it and I'm happy to do this either way.

Thanks,
Günther

--

Mickaël Salaün Sept. 30, 2022, 3:56 p.m. UTC | #11

On 29/09/2022 21:22, Günther Noack wrote:
> On Wed, Sep 28, 2022 at 08:32:02PM +0200, Mickaël Salaün wrote:
>> On 25/09/2022 20:09, Günther Noack wrote:
>>> On Fri, Sep 23, 2022 at 10:53:23PM +0200, Mickaël Salaün wrote:
>>>> On 23/09/2022 13:21, Günther Noack wrote:
>>>>> On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
>>>>>> On 08/09/2022 21:58, Günther Noack wrote:
>>>>>>> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +/**
>>>>>>> + * get_path_access_rights - Returns the subset of rights in access_request
>>>>>>> + * which are permitted for the given path.
>>>>>>> + *
>>>>>>> + * @domain: The domain that defines the current restrictions.
>>>>>>> + * @path: The path to get access rights for.
>>>>>>> + * @access_request: The rights we are interested in.
>>>>>>> + *
>>>>>>> + * Returns: The access mask of the rights that are permitted on the given path,
>>>>>>> + * which are also a subset of access_request (to save some calculation time).
>>>>>>> + */
>>>>>>> +static inline access_mask_t
>>>>>>> +get_path_access_rights(const struct landlock_ruleset *const domain,
>>>>>>> +		       const struct path *const path,
>>>>>>> +		       access_mask_t access_request)
>>>>>>> +{
>>>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>>>>>> +	unsigned long access_bit;
>>>>>>> +	unsigned long access_req;
>>>>>>> +
>>>>>>> +	init_layer_masks(domain, access_request, &layer_masks);
>>>>>>> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
>>>>>>> +				    NULL, 0, NULL, NULL)) {
>>>>>>> +		/*
>>>>>>> +		 * Return immediately for successful accesses and for cases
>>>>>>> +		 * where everything is permitted because the path belongs to an
>>>>>>> +		 * internal filesystem.
>>>>>>> +		 */
>>>>>>> +		return access_request;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	access_req = access_request;
>>>>>>> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
>>>>>>> +		if (layer_masks[access_bit]) {
>>>>>>> +			/* If any layer vetoed the access right, remove it. */
>>>>>>> +			access_request &= ~BIT_ULL(access_bit);
>>>>>>> +		}
>>>>>>> +	}
>>>>>>
>>>>>> This seems to be redundant with the value returned by init_layer_masks(),
>>>>>> which should be passed to check_access_path_dual() to avoid useless path
>>>>>> walk.
>>>>>
>>>>> True, I'll use the result of init_layer_masks() to feed it back to
>>>>> check_access_path_dual() to avoid a bit of computation.
>>>>>
>>>>> Like this:
>>>>>
>>>>>            effective_access_request =
>>>>> 		init_layer_masks(domain, access_request, &layer_masks);
>>>>> 	if (!check_access_path_dual(domain, path, effective_access_request,
>>>>> 	    &layer_masks, NULL, 0, NULL, NULL)) {
>>>>> 		// ...
>>>>> 	}
>>>>
>>>> correct
>>>>
>>>>>
>>>>> Overall, the approach here is:
>>>>>
>>>>> * Initialize the layer_masks, so that it has a bit set for every
>>>>>      access right in access_request and layer where that access right is
>>>>>      handled.
>>>>>
>>>>> * check_access_path_dual() with only the first few parameters -- this
>>>>>      will clear all the bits in layer masks which are actually permitted
>>>>>      according to the individual rules.
>>>>>
>>>>>      As a special case, this *may* return 0 immediately, in which case we
>>>>>      can (a) save a bit of calculation in the loop below and (b) we might
>>>>>      be in the case where access is permitted because it's a file from a
>>>>>      special file system (even though not all bits are cleared). If
>>>>>      check_access_path_dual() returns 0, we return the full requested
>>>>>      access_request that we received as input. >
>>>>> * In the loop below, if there are any bits left in layer_masks, those
>>>>>      are rights which are not permitted for the given path. We remove
>>>>>      these from access_request and return the modified access_request.
>>>>>
>>>>>
>>>>>> This function is pretty similar to check_access_path(). Can't you change it
>>>>>> to use an access_mask_t pointer and get almost the same thing?
>>>>>
>>>>> I'm shying away from this approach. Many of the existing different use
>>>>> cases are already realized by "doing if checks deep down". I think it
>>>>> would make the code more understandable if we managed to model these
>>>>> differences between use cases already at the layer of function calls.
>>>>> (This is particularly true for check_access_path_dual(), where in
>>>>> order to find out how the "single" case works, you need to disentangle
>>>>> to a large extent how the much more complicated dual case works.)
>>>>
>>>> I agree that check_access_path_dual() is complex, but I couldn't find a
>>>> better way.
>>>
>>> It seems out of the scope of this patch set, but I sometimes find it
>>> OK to just duplicate the code and have a set of tests to demonstrate
>>> that the two variants do the same thing.
>>>
>>> check_access_path_dual() is mostly complex because of performance
>>> reasons, as far as I can tell, and it might be possible to check its
>>> results against a parallel implementation of it which runs slower,
>>> uses more memory, but is more obviously correct. (I have used one
>>> myself to check against when developing the truncate patch set.)
>>>
>>>>> If you want to unify these two functions, what do you think of the
>>>>> approach of just using get_path_access_rights() instead of
>>>>> check_access_path()?
>>>>>
>>>>> Basically, it would turn
>>>>>
>>>>> return check_access_path(dom, path, access_request);
>>>>>
>>>>> into
>>>>>
>>>>> if (get_path_access_rights(dom, path, access_request) == access_request)
>>>>> 	return 0;
>>>>> return -EACCES;
>>>>>
>>>>> This is slightly more verbose in the places where it's called, but it
>>>>> would be more orthogonal, and it would also clarify that -EACCES is
>>>>> the only possible error in the "single" path walk case.
>>>>>
>>>>> Let me know what you think.
>>>>
>>>> What about adding an additional argument `access_mask_t *const
>>>> access_allowed` to check_access_path_dual() which returns the set of
>>>> accesses (i.e. access_masked_parent1 & access_masked_parent2) that could
>>>> then be stored to landlock_file(file)->allowed_access? If this argument is
>>>> NULL it should just be ignored. What is left from get_path_access_rights()
>>>> could then be merged into hook_file_open().
>>>
>>> IMHO, check_access_path_dual() does not seem like the right place to
>>> add this. This functionality is not needed in any of the "dual path"
>>> cases so far, and I'm not sure what it would mean. The necessary
>>> information can also be easily derived from the resulting layer_masks,
>>> which is already exposed in the check_access_path_dual() interface,
>>> and I also believe that this approach is at least equally fast as
>>> updating it on the fly when changing the layer_masks.
>>>
>>> I could be convinced to add a `access_mask_t *const access_allowed`
>>> argument to check_access_path() if you prefer that, but then again, in
>>> that case the returned boolean can be reconstructed from the new
>>> access_allowed variable, and we could as well make check_access_path()
>>> return the access_allowed result instead of the boolean and let
>>> callers check equality with what they expected...? (I admittedly don't
>>> have a good setup to test the performance right now, but it looks like
>>> a negligible difference to me?)
>>
>> Good idea, let's try to make check_access_path_dual() returns the allowed
>> accesses (according to the request) and rename it to get_access_path_dual().
>> unmask_layers() could be changed to return the still-denied accesses instead
>> of a boolean, and we could use this values (for potential both parents) to
>> return allowed_parent1 & allowed_parent2 (with access_mask_t types). This
>> would also simplify is_eaccess() and its calls could be moved to
>> current_check_refer_path(). This would merge get_path_access_rights() into
>> check_access_path_dual() and make the errno codes more explicit per hook or
>> defined in check_access_path().
> 
> Thanks for the review!
> 
> I'm afraid I don't understand this approach at the moment. I'm
> probably still missing some insight about how the "refer" logic works
> which would make this clearer.
> 
> With the proposed changes to check_access_path_dual(), it sounds like
> we would have to change the logic of the "refer" implementation quite
> a bit, which would expand the scope of the "truncate" patch set beyond
> what it was originally meant to do. Is this check_access_path_dual()
> refactoring something you'd insist on for the truncate patch set, or
> would you be OK with doing that separately?

I'd like to avoid stacking debts and I prefer to refactor code instead, 
but I got your point. Here is another proposal closer to yours. Let's 
rename check_access_path_dual() to is_access_to_paths_allowed(), make it 
returns a boolean (allowed_parent1 && allowed_parent2), and move the 
EACCES/EXDEV logic to (only) after the second call to 
check_access_path_dual() by current_check_refer_path() (because the if 
(old_dentry->d_parent == new_dir->dentry) branch cannot return EXDEV).

check_access_path() either returns 0 or -EACCES, and we should add a 
WARN_ON_ONCE(access_request & LANDLOCK_ACCESS_FS_REFER) to make sure 
this remains correct.

The get_path_access_rights() logic can be moved to hook_file_open() to 
make it more readable.


> 
> For the truncate patch set, what do you think of the lighter
> refactoring options, which I had outlined in my previous mail? - see
> the four bullet points quoted here:
> 
>>> Here are the options we have discussed, in the order that I would
>>> prefer them:
>>>
>>> * to keep it as a separate function as it already is,
>>>     slightly duplicating check_access_path(). (I think it's cleaner,
>>>     because the code path for the rest of the hooks other than
>>>     security_file_open() stays simpler.)
>>>
>>> * to make check_access_path() return the access_allowed access mask
>>>     and make callers check that it covers the access_request that they
>>>     asked for (see example from my previous mail on this thread). (This
>>>     is equivalent to discarding the existing check_access_path() and
>>>     using the get_path_access() function instead.)
>>>
>>> * to add a `access_mask_t *const access_allowed` argument to
>>>     check_access_path(), which is calculated if it's non-NULL based on
>>>     the layer_masks result. It would be used from the security_file_open
>>>     hook.
>>>
>>> * to add a `access_mask_t *const access_allowed` argument to
>>>     check_access_path_dual(). This doesn't make much sense, IMHO,
>>>     because an on-the-fly calculation of this result does not look like
>>>     a performance benefit to me, and calculating it based on the two
>>>     resulting layer_masks is already possible now. It's also not clear
>>>     to me what it would mean to calculate an access_allowed on two paths
>>>     at once, and what that would be used for.
>>>
>>> Let me know which option you prefer. In the end, I don't feel that
>>> strongly about it and I'm happy to do this either way.
> 
> Thanks,
> Günther
>

Patch

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..8c0124c5cbe6 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -95,8 +95,16 @@  struct landlock_path_beneath_attr {
  * A file can only receive these access rights:
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
- * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
+ * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
+ *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
+ *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
+ *   :manpage:`creat(2)`.
  * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
+ * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
+ *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
+ *   `O_TRUNC`. The right to truncate a file gets carried along with an opened
+ *   file descriptor for the purpose of :manpage:`ftruncate(2)`. This access
+ *   right is available since the third version of the Landlock ABI.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -139,10 +147,9 @@  struct landlock_path_beneath_attr {
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
- *   :manpage:`truncate(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:`chmod(2)`,
+ *   :manpage:`chown(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 */
@@ -160,6 +167,7 @@  struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
+#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index a9dbd99d9ee7..1b546edf69a6 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -146,7 +146,8 @@  static struct landlock_object *get_inode_object(struct inode *const inode)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 /* clang-format on */
 
 /*
@@ -761,6 +762,47 @@  static bool collect_domain_accesses(
 	return ret;
 }
 
+/**
+ * get_path_access_rights - Returns the subset of rights in access_request
+ * which are permitted for the given path.
+ *
+ * @domain: The domain that defines the current restrictions.
+ * @path: The path to get access rights for.
+ * @access_request: The rights we are interested in.
+ *
+ * Returns: The access mask of the rights that are permitted on the given path,
+ * which are also a subset of access_request (to save some calculation time).
+ */
+static inline access_mask_t
+get_path_access_rights(const struct landlock_ruleset *const domain,
+		       const struct path *const path,
+		       access_mask_t access_request)
+{
+	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
+	unsigned long access_bit;
+	unsigned long access_req;
+
+	init_layer_masks(domain, access_request, &layer_masks);
+	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
+				    NULL, 0, NULL, NULL)) {
+		/*
+		 * Return immediately for successful accesses and for cases
+		 * where everything is permitted because the path belongs to an
+		 * internal filesystem.
+		 */
+		return access_request;
+	}
+
+	access_req = access_request;
+	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
+		if (layer_masks[access_bit]) {
+			/* If any layer vetoed the access right, remove it. */
+			access_request &= ~BIT_ULL(access_bit);
+		}
+	}
+	return access_request;
+}
+
 /**
  * current_check_refer_path - Check if a rename or link action is allowed
  *
@@ -1142,6 +1184,11 @@  static int hook_path_rmdir(const struct path *const dir,
 	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
 }
 
+static int hook_path_truncate(const struct path *const path)
+{
+	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
+}
+
 /* File hooks */
 
 static inline access_mask_t get_file_access(const struct file *const file)
@@ -1159,22 +1206,55 @@  static inline access_mask_t get_file_access(const struct file *const file)
 	/* __FMODE_EXEC is indeed part of f_flags, not f_mode. */
 	if (file->f_flags & __FMODE_EXEC)
 		access |= LANDLOCK_ACCESS_FS_EXECUTE;
+
 	return access;
 }
 
 static int hook_file_open(struct file *const file)
 {
+	access_mask_t access_req, access_rights;
+	const access_mask_t optional_rights = LANDLOCK_ACCESS_FS_TRUNCATE;
 	const struct landlock_ruleset *const dom =
 		landlock_get_current_domain();
 
-	if (!dom)
+	if (!dom) {
+		/* Grant all rights. */
+		landlock_file(file)->rights = LANDLOCK_MASK_ACCESS_FS;
 		return 0;
+	}
+
 	/*
 	 * Because a file may be opened with O_PATH, get_file_access() may
 	 * return 0.  This case will be handled with a future Landlock
 	 * evolution.
 	 */
-	return check_access_path(dom, &file->f_path, get_file_access(file));
+	access_req = get_file_access(file);
+	access_rights = get_path_access_rights(dom, &file->f_path,
+					       access_req | optional_rights);
+	if (access_req & ~access_rights)
+		return -EACCES;
+
+	/*
+	 * For operations on already opened files (i.e. ftruncate()), it is the
+	 * access rights at the time of open() which decide whether the
+	 * operation is permitted. Therefore, we record the relevant subset of
+	 * file access rights in the opened struct file.
+	 */
+	landlock_file(file)->rights = access_rights;
+
+	return 0;
+}
+
+static int hook_file_truncate(struct file *const file)
+{
+	/*
+	 * We permit truncation if the truncation right was available at the
+	 * time of opening the file.
+	 */
+	if (!(landlock_file(file)->rights & LANDLOCK_ACCESS_FS_TRUNCATE))
+		return -EACCES;
+
+	return 0;
 }
 
 static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
@@ -1194,6 +1274,8 @@  static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
 	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(file_truncate, hook_file_truncate),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/fs.h b/security/landlock/fs.h
index 8db7acf9109b..275ba5375839 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h
@@ -36,6 +36,18 @@  struct landlock_inode_security {
 	struct landlock_object __rcu *object;
 };
 
+/**
+ * struct landlock_file_security - File security blob
+ *
+ * This information is populated when opening a file in hook_file_open, and
+ * tracks the relevant Landlock access rights that were available at the time
+ * of opening the file. Other LSM hooks use these rights in order to authorize
+ * operations on already opened files.
+ */
+struct landlock_file_security {
+	access_mask_t rights;
+};
+
 /**
  * struct landlock_superblock_security - Superblock security blob
  *
@@ -50,6 +62,12 @@  struct landlock_superblock_security {
 	atomic_long_t inode_refs;
 };
 
+static inline struct landlock_file_security *
+landlock_file(const struct file *const file)
+{
+	return file->f_security + landlock_blob_sizes.lbs_file;
+}
+
 static inline struct landlock_inode_security *
 landlock_inode(const struct inode *const inode)
 {
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index b54184ab9439..82288f0e9e5e 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_REFER
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
 #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/setup.c b/security/landlock/setup.c
index f8e8e980454c..3f196d2ce4f9 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -19,6 +19,7 @@  bool landlock_initialized __lsm_ro_after_init = false;
 
 struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
 	.lbs_cred = sizeof(struct landlock_cred_security),
+	.lbs_file = sizeof(struct landlock_file_security),
 	.lbs_inode = sizeof(struct landlock_inode_security),
 	.lbs_superblock = sizeof(struct landlock_superblock_security),
 };
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 735a0865ea11..f4d6fc7ed17f 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 2
+#define LANDLOCK_ABI_VERSION 3
 
 /**
  * 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 da9290817866..72cdae277b02 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(2, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(3, 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 45de42a027c5..87b28d14a1aa 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -406,9 +406,10 @@  TEST_F_FORK(layout1, inval)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -422,7 +423,7 @@  TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	ACCESS_LAST)
+	LANDLOCK_ACCESS_FS_REFER)
 
 /* clang-format on */