| Message ID | 20221013223654.659758-6-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | integrity: Move hooks into LSM | expand |
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.
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.
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;
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 --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;
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(-)