diff mbox series

[v1,05/11] landlock: Move filesystem helpers and add a new one

Message ID 20220221212522.320243-6-mic@digikod.net (mailing list archive)
State New, archived
Headers show
Series Landlock: file linking and renaming support | expand

Commit Message

Mickaël Salaün Feb. 21, 2022, 9:25 p.m. UTC 
From: Mickaël Salaün <mic@linux.microsoft.com>

Move the SB_NOUSER and IS_PRIVATE dentry check to a standalone
is_nouser_or_private() helper.  This will be useful for a following
commit.

Move get_mode_access() and maybe_remove() to make them usable by new
code provided by a following commit.

Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20220221212522.320243-6-mic@digikod.net
---
 security/landlock/fs.c | 87 ++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 41 deletions(-)

Comments

Paul Moore March 17, 2022, 1:26 a.m. UTC | #1 
On Mon, Feb 21, 2022 at 4:15 PM Mickaël Salaün <mic@digikod.net> wrote:
>
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Move the SB_NOUSER and IS_PRIVATE dentry check to a standalone
> is_nouser_or_private() helper.  This will be useful for a following
> commit.
>
> Move get_mode_access() and maybe_remove() to make them usable by new
> code provided by a following commit.
>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20220221212522.320243-6-mic@digikod.net
> ---
>  security/landlock/fs.c | 87 ++++++++++++++++++++++--------------------
>  1 file changed, 46 insertions(+), 41 deletions(-)

One nit-picky comment below, otherwise it looks fine to me.

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

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 9662f9fb3cd0..3886f9ad1a60 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -257,6 +257,18 @@ static inline bool unmask_layers(const struct landlock_rule *const rule,
>         return false;
>  }
>
> +static inline bool is_nouser_or_private(const struct dentry *dentry)
> +{
> +       /*
> +        * Allows access to pseudo filesystems that will never be mountable
> +        * (e.g. sockfs, pipefs), but can still be reachable through
> +        * /proc/<pid>/fd/<file-descriptor> .
> +        */

I might suggest moving this explanation up to a function header comment block.


> +       return (dentry->d_sb->s_flags & SB_NOUSER) ||
> +                       (d_is_positive(dentry) &&
> +                        unlikely(IS_PRIVATE(d_backing_inode(dentry))));
> +}
> +
>  static int check_access_path(const struct landlock_ruleset *const domain,
>                 const struct path *const path,
>                 const access_mask_t access_request)

--
paul-moore.com
Mickaël Salaün March 17, 2022, 10:42 a.m. UTC | #2 
On 17/03/2022 02:26, Paul Moore wrote:
> On Mon, Feb 21, 2022 at 4:15 PM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>
>> Move the SB_NOUSER and IS_PRIVATE dentry check to a standalone
>> is_nouser_or_private() helper.  This will be useful for a following
>> commit.
>>
>> Move get_mode_access() and maybe_remove() to make them usable by new
>> code provided by a following commit.
>>
>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>> Link: https://lore.kernel.org/r/20220221212522.320243-6-mic@digikod.net
>> ---
>>   security/landlock/fs.c | 87 ++++++++++++++++++++++--------------------
>>   1 file changed, 46 insertions(+), 41 deletions(-)
> 
> One nit-picky comment below, otherwise it looks fine to me.
> 
> Reviewed-by: Paul Moore <paul@paul-moore.com>
> 
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 9662f9fb3cd0..3886f9ad1a60 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -257,6 +257,18 @@ static inline bool unmask_layers(const struct landlock_rule *const rule,
>>          return false;
>>   }
>>
>> +static inline bool is_nouser_or_private(const struct dentry *dentry)
>> +{
>> +       /*
>> +        * Allows access to pseudo filesystems that will never be mountable
>> +        * (e.g. sockfs, pipefs), but can still be reachable through
>> +        * /proc/<pid>/fd/<file-descriptor> .
>> +        */
> 
> I might suggest moving this explanation up to a function header comment block.

Sounds good.


> 
> 
>> +       return (dentry->d_sb->s_flags & SB_NOUSER) ||
>> +                       (d_is_positive(dentry) &&
>> +                        unlikely(IS_PRIVATE(d_backing_inode(dentry))));
>> +}
>> +
>>   static int check_access_path(const struct landlock_ruleset *const domain,
>>                  const struct path *const path,
>>                  const access_mask_t access_request)
> 
> --
> paul-moore.com
diff mbox series

Patch

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 9662f9fb3cd0..3886f9ad1a60 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -257,6 +257,18 @@  static inline bool unmask_layers(const struct landlock_rule *const rule,
 	return false;
 }
 
+static inline bool is_nouser_or_private(const struct dentry *dentry)
+{
+	/*
+	 * Allows access to pseudo filesystems that will never be mountable
+	 * (e.g. sockfs, pipefs), but can still be reachable through
+	 * /proc/<pid>/fd/<file-descriptor> .
+	 */
+	return (dentry->d_sb->s_flags & SB_NOUSER) ||
+			(d_is_positive(dentry) &&
+			 unlikely(IS_PRIVATE(d_backing_inode(dentry))));
+}
+
 static int check_access_path(const struct landlock_ruleset *const domain,
 		const struct path *const path,
 		const access_mask_t access_request)
@@ -270,14 +282,7 @@  static int check_access_path(const struct landlock_ruleset *const domain,
 		return 0;
 	if (WARN_ON_ONCE(!domain || !path))
 		return 0;
-	/*
-	 * Allows access to pseudo filesystems that will never be mountable
-	 * (e.g. sockfs, pipefs), but can still be reachable through
-	 * /proc/<pid>/fd/<file-descriptor> .
-	 */
-	if ((path->dentry->d_sb->s_flags & SB_NOUSER) ||
-			(d_is_positive(path->dentry) &&
-			 unlikely(IS_PRIVATE(d_backing_inode(path->dentry)))))
+	if (is_nouser_or_private(path->dentry))
 		return 0;
 	if (WARN_ON_ONCE(domain->num_layers < 1))
 		return -EACCES;
@@ -356,6 +361,39 @@  static inline int current_check_access_path(const struct path *const path,
 	return check_access_path(dom, path, access_request);
 }
 
+static inline access_mask_t get_mode_access(const umode_t mode)
+{
+	switch (mode & S_IFMT) {
+	case S_IFLNK:
+		return LANDLOCK_ACCESS_FS_MAKE_SYM;
+	case 0:
+		/* A zero mode translates to S_IFREG. */
+	case S_IFREG:
+		return LANDLOCK_ACCESS_FS_MAKE_REG;
+	case S_IFDIR:
+		return LANDLOCK_ACCESS_FS_MAKE_DIR;
+	case S_IFCHR:
+		return LANDLOCK_ACCESS_FS_MAKE_CHAR;
+	case S_IFBLK:
+		return LANDLOCK_ACCESS_FS_MAKE_BLOCK;
+	case S_IFIFO:
+		return LANDLOCK_ACCESS_FS_MAKE_FIFO;
+	case S_IFSOCK:
+		return LANDLOCK_ACCESS_FS_MAKE_SOCK;
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
+static inline access_mask_t maybe_remove(const struct dentry *const dentry)
+{
+	if (d_is_negative(dentry))
+		return 0;
+	return d_is_dir(dentry) ? LANDLOCK_ACCESS_FS_REMOVE_DIR :
+		LANDLOCK_ACCESS_FS_REMOVE_FILE;
+}
+
 /* Inode hooks */
 
 static void hook_inode_free_security(struct inode *const inode)
@@ -549,31 +587,6 @@  static int hook_sb_pivotroot(const struct path *const old_path,
 
 /* Path hooks */
 
-static inline access_mask_t get_mode_access(const umode_t mode)
-{
-	switch (mode & S_IFMT) {
-	case S_IFLNK:
-		return LANDLOCK_ACCESS_FS_MAKE_SYM;
-	case 0:
-		/* A zero mode translates to S_IFREG. */
-	case S_IFREG:
-		return LANDLOCK_ACCESS_FS_MAKE_REG;
-	case S_IFDIR:
-		return LANDLOCK_ACCESS_FS_MAKE_DIR;
-	case S_IFCHR:
-		return LANDLOCK_ACCESS_FS_MAKE_CHAR;
-	case S_IFBLK:
-		return LANDLOCK_ACCESS_FS_MAKE_BLOCK;
-	case S_IFIFO:
-		return LANDLOCK_ACCESS_FS_MAKE_FIFO;
-	case S_IFSOCK:
-		return LANDLOCK_ACCESS_FS_MAKE_SOCK;
-	default:
-		WARN_ON_ONCE(1);
-		return 0;
-	}
-}
-
 /*
  * Creating multiple links or renaming may lead to privilege escalations if not
  * handled properly.  Indeed, we must be sure that the source doesn't gain more
@@ -601,14 +614,6 @@  static int hook_path_link(struct dentry *const old_dentry,
 			get_mode_access(d_backing_inode(old_dentry)->i_mode));
 }
 
-static inline access_mask_t maybe_remove(const struct dentry *const dentry)
-{
-	if (d_is_negative(dentry))
-		return 0;
-	return d_is_dir(dentry) ? LANDLOCK_ACCESS_FS_REMOVE_DIR :
-		LANDLOCK_ACCESS_FS_REMOVE_FILE;
-}
-
 static int hook_path_rename(const struct path *const old_dir,
 		struct dentry *const old_dentry,
 		const struct path *const new_dir,