diff mbox series

[6/9] fs: Introduce file_to_perms() helper

Message ID 20221013223654.659758-6-keescook@chromium.org
State New
Headers
Series integrity: Move hooks into LSM |

Commit Message

Kees Cook Oct. 13, 2022, 10:36 p.m. UTC 
  Extract the logic used by LSM file hooks to be able to reconstruct the
access mode permissions from an open.

Cc: John Johansen <john.johansen@canonical.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/fs.h               | 22 ++++++++++++++++++++++
 security/apparmor/include/file.h | 18 ++++--------------
 2 files changed, 26 insertions(+), 14 deletions(-)

Comments

Christian Brauner Oct. 18, 2022, 2:10 p.m. UTC | #1 
On Thu, Oct 13, 2022 at 03:36:51PM -0700, Kees Cook wrote:
> Extract the logic used by LSM file hooks to be able to reconstruct the
> access mode permissions from an open.
> 
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/fs.h               | 22 ++++++++++++++++++++++
>  security/apparmor/include/file.h | 18 ++++--------------
>  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9eced4cc286e..814f10d4132e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
>  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
>  #define file_count(x)	atomic_long_read(&(x)->f_count)
>  
> +/* Calculate the basic MAY_* flags needed for a given file. */
> +static inline u8 file_to_perms(struct file *file)

As long as there aren't multiple users of this and especially none in
the vfs proper please don't move this into fs.h. It's overloaded enough
as it is and we have vague plans on splitting it further in the future.
Kees Cook Oct. 18, 2022, 6:25 p.m. UTC | #2 
On Tue, Oct 18, 2022 at 04:10:37PM +0200, Christian Brauner wrote:
> On Thu, Oct 13, 2022 at 03:36:51PM -0700, Kees Cook wrote:
> > Extract the logic used by LSM file hooks to be able to reconstruct the
> > access mode permissions from an open.
> > 
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  include/linux/fs.h               | 22 ++++++++++++++++++++++
> >  security/apparmor/include/file.h | 18 ++++--------------
> >  2 files changed, 26 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9eced4cc286e..814f10d4132e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
> >  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
> >  #define file_count(x)	atomic_long_read(&(x)->f_count)
> >  
> > +/* Calculate the basic MAY_* flags needed for a given file. */
> > +static inline u8 file_to_perms(struct file *file)
> 
> As long as there aren't multiple users of this and especially none in
> the vfs proper please don't move this into fs.h. It's overloaded enough
> as it is and we have vague plans on splitting it further in the future.

Sure -- this can live in a security .h file somewhere.
Casey Schaufler Oct. 20, 2022, 5:29 p.m. UTC | #3 
On 10/13/2022 3:36 PM, Kees Cook wrote:
> Extract the logic used by LSM file hooks to be able to reconstruct the
> access mode permissions from an open.
>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/fs.h               | 22 ++++++++++++++++++++++
>  security/apparmor/include/file.h | 18 ++++--------------
>  2 files changed, 26 insertions(+), 14 deletions(-)

Smack uses its own definitions for MAY_SOMETHING. Making
AppArmor's values global is going to clash. If you want to
do this there needs to be a grand consolidation. It could
go in security/lsm_hooks.h. I can't see anyone other than
Smack wanting MAY_LOCK, so I can't say the concept really
makes much sense.

>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9eced4cc286e..814f10d4132e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
>  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
>  #define file_count(x)	atomic_long_read(&(x)->f_count)
>  
> +/* Calculate the basic MAY_* flags needed for a given file. */
> +static inline u8 file_to_perms(struct file *file)
> +{
> +	__auto_type flags = file->f_flags;
> +	unsigned int perms = 0;
> +
> +	if (file->f_mode & FMODE_EXEC)
> +		perms |= MAY_EXEC;
> +	if (file->f_mode & FMODE_WRITE)
> +		perms |= MAY_WRITE;
> +	if (file->f_mode & FMODE_READ)
> +		perms |= MAY_READ;
> +	if ((flags & O_APPEND) && (perms & MAY_WRITE))
> +		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
> +	/* trunc implies write permission */
> +	if (flags & O_TRUNC)
> +		perms |= MAY_WRITE;
> +
> +	/* We must only return the basic permissions low-nibble perms. */
> +	return (perms | (MAY_EXEC | MAY_WRITE | MAY_READ | MAY_APPEND));
> +}
> +
>  #define	MAX_NON_LFS	((1UL<<31) - 1)
>  
>  /* Page cache limit. The filesystems should put that into their s_maxbytes 
> diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
> index 029cb20e322d..505d6da02af3 100644
> --- a/security/apparmor/include/file.h
> +++ b/security/apparmor/include/file.h
> @@ -218,20 +218,10 @@ static inline void aa_free_file_rules(struct aa_file_rules *rules)
>   */
>  static inline u32 aa_map_file_to_perms(struct file *file)
>  {
> -	int flags = file->f_flags;
> -	u32 perms = 0;
> -
> -	if (file->f_mode & FMODE_WRITE)
> -		perms |= MAY_WRITE;
> -	if (file->f_mode & FMODE_READ)
> -		perms |= MAY_READ;
> -
> -	if ((flags & O_APPEND) && (perms & MAY_WRITE))
> -		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
> -	/* trunc implies write permission */
> -	if (flags & O_TRUNC)
> -		perms |= MAY_WRITE;
> -	if (flags & O_CREAT)
> +	u32 perms = file_to_perms(file);
> +
> +	/* Also want to check O_CREAT */
> +	if (file->f_flags & O_CREAT)
>  		perms |= AA_MAY_CREATE;
>  
>  	return perms;
Kees Cook Oct. 20, 2022, 11:04 p.m. UTC | #4 
On Thu, Oct 20, 2022 at 10:29:08AM -0700, Casey Schaufler wrote:
> On 10/13/2022 3:36 PM, Kees Cook wrote:
> > Extract the logic used by LSM file hooks to be able to reconstruct the
> > access mode permissions from an open.
> >
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  include/linux/fs.h               | 22 ++++++++++++++++++++++
> >  security/apparmor/include/file.h | 18 ++++--------------
> >  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> Smack uses its own definitions for MAY_SOMETHING. Making
> AppArmor's values global is going to clash. If you want to
> do this there needs to be a grand consolidation. It could
> go in security/lsm_hooks.h. I can't see anyone other than
> Smack wanting MAY_LOCK, so I can't say the concept really
> makes much sense.

I left AppArmor's special ones in apparmor/. This only lifts the common
pre-existing global VFS MAY_* flags. (And only the low nibble's worth).
diff mbox series

Patch

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9eced4cc286e..814f10d4132e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -993,6 +993,28 @@  static inline struct file *get_file(struct file *f)
 #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
 #define file_count(x)	atomic_long_read(&(x)->f_count)
 
+/* Calculate the basic MAY_* flags needed for a given file. */
+static inline u8 file_to_perms(struct file *file)
+{
+	__auto_type flags = file->f_flags;
+	unsigned int perms = 0;
+
+	if (file->f_mode & FMODE_EXEC)
+		perms |= MAY_EXEC;
+	if (file->f_mode & FMODE_WRITE)
+		perms |= MAY_WRITE;
+	if (file->f_mode & FMODE_READ)
+		perms |= MAY_READ;
+	if ((flags & O_APPEND) && (perms & MAY_WRITE))
+		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
+	/* trunc implies write permission */
+	if (flags & O_TRUNC)
+		perms |= MAY_WRITE;
+
+	/* We must only return the basic permissions low-nibble perms. */
+	return (perms | (MAY_EXEC | MAY_WRITE | MAY_READ | MAY_APPEND));
+}
+
 #define	MAX_NON_LFS	((1UL<<31) - 1)
 
 /* Page cache limit. The filesystems should put that into their s_maxbytes 
diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
index 029cb20e322d..505d6da02af3 100644
--- a/security/apparmor/include/file.h
+++ b/security/apparmor/include/file.h
@@ -218,20 +218,10 @@  static inline void aa_free_file_rules(struct aa_file_rules *rules)
  */
 static inline u32 aa_map_file_to_perms(struct file *file)
 {
-	int flags = file->f_flags;
-	u32 perms = 0;
-
-	if (file->f_mode & FMODE_WRITE)
-		perms |= MAY_WRITE;
-	if (file->f_mode & FMODE_READ)
-		perms |= MAY_READ;
-
-	if ((flags & O_APPEND) && (perms & MAY_WRITE))
-		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
-	/* trunc implies write permission */
-	if (flags & O_TRUNC)
-		perms |= MAY_WRITE;
-	if (flags & O_CREAT)
+	u32 perms = file_to_perms(file);
+
+	/* Also want to check O_CREAT */
+	if (file->f_flags & O_CREAT)
 		perms |= AA_MAY_CREATE;
 
 	return perms;