fs: Pass AT_GETATTR_NOSEC flag to getattr interface function

Commit Message

Stefan Berger Oct. 2, 2023, 12:57 p.m. UTC

From: Stefan Berger <[e-mail]>

When vfs_getattr_nosec() calls a filesystem's getattr interface function
then the 'nosec' should propagate into this function so that
vfs_getattr_nosec() can again be called from the filesystem's gettattr
rather than vfs_getattr(). The latter would add unnecessary security
checks that the initial vfs_getattr_nosec() call wanted to avoid.
Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
with the new getattr_flags parameter to the getattr interface function.
In overlayfs and ecryptfs use this flag to determine which one of the
two functions to call.

In a recent code change introduced to IMA vfs_getattr_nosec() ended up
calling vfs_getattr() in overlayfs, which in turn called
security_inode_getattr() on an exiting process that did not have
current->fs set anymore, which then caused a kernel NULL pointer
dereference. With this change the call to security_inode_getattr() can
be avoided, thus avoiding the NULL pointer dereference.

Reported-by: [e-mail]
Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
Cc: Alexander Viro <[e-mail]>
Cc: [e-mail]
Cc: Miklos Szeredi <[e-mail]>
Cc: Amir Goldstein <[e-mail]>
Cc: Tyler Hicks <[e-mail]>
Cc: Mimi Zohar <[e-mail]>
Suggested-by: Christian Brauner <[e-mail]>
Co-developed-by: Amir Goldstein <[e-mail]>
Signed-off-by: Stefan Berger <[e-mail]>
---
 fs/ecryptfs/inode.c        | 12 ++++++++++--
 fs/overlayfs/inode.c       | 10 +++++-----
 fs/overlayfs/overlayfs.h   |  8 ++++++++
 fs/stat.c                  |  6 +++++-
 include/uapi/linux/fcntl.h |  3 +++
 5 files changed, 31 insertions(+), 8 deletions(-)

Comments

Amir Goldstein Oct. 2, 2023, 1:22 p.m. UTC | #1

On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[e-mail]> wrote:
>
> From: Stefan Berger <[e-mail]>
>
> When vfs_getattr_nosec() calls a filesystem's getattr interface function
> then the 'nosec' should propagate into this function so that
> vfs_getattr_nosec() can again be called from the filesystem's gettattr
> rather than vfs_getattr(). The latter would add unnecessary security
> checks that the initial vfs_getattr_nosec() call wanted to avoid.
> Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
> with the new getattr_flags parameter to the getattr interface function.
> In overlayfs and ecryptfs use this flag to determine which one of the
> two functions to call.
>
> In a recent code change introduced to IMA vfs_getattr_nosec() ended up
> calling vfs_getattr() in overlayfs, which in turn called
> security_inode_getattr() on an exiting process that did not have
> current->fs set anymore, which then caused a kernel NULL pointer
> dereference. With this change the call to security_inode_getattr() can
> be avoided, thus avoiding the NULL pointer dereference.
>
> Reported-by: [e-mail]
> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> Cc: Alexander Viro <[e-mail]>
> Cc: [e-mail]
> Cc: Miklos Szeredi <[e-mail]>
> Cc: Amir Goldstein <[e-mail]>
> Cc: Tyler Hicks <[e-mail]>
> Cc: Mimi Zohar <[e-mail]>
> Suggested-by: Christian Brauner <[e-mail]>
> Co-developed-by: Amir Goldstein <[e-mail]>
> Signed-off-by: Stefan Berger <[e-mail]>
> ---

Reviewed-by: Amir Goldstein <[e-mail]>

Now let's see what vfs maintainers think about this...

Thanks,
Amir.

>  fs/ecryptfs/inode.c        | 12 ++++++++++--
>  fs/overlayfs/inode.c       | 10 +++++-----
>  fs/overlayfs/overlayfs.h   |  8 ++++++++
>  fs/stat.c                  |  6 +++++-
>  include/uapi/linux/fcntl.h |  3 +++
>  5 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 992d9c7e64ae..5ab4b87888a7 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -998,6 +998,14 @@ static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
>         return rc;
>  }
>
> +static int ecryptfs_do_getattr(const struct path *path, struct kstat *stat,
> +                              u32 request_mask, unsigned int flags)
> +{
> +       if (flags & AT_GETATTR_NOSEC)
> +               return vfs_getattr_nosec(path, stat, request_mask, flags);
> +       return vfs_getattr(path, stat, request_mask, flags);
> +}
> +
>  static int ecryptfs_getattr(struct mnt_idmap *idmap,
>                             const struct path *path, struct kstat *stat,
>                             u32 request_mask, unsigned int flags)
> @@ -1006,8 +1014,8 @@ static int ecryptfs_getattr(struct mnt_idmap *idmap,
>         struct kstat lower_stat;
>         int rc;
>
> -       rc = vfs_getattr(ecryptfs_dentry_to_lower_path(dentry), &lower_stat,
> -                        request_mask, flags);
> +       rc = ecryptfs_do_getattr(ecryptfs_dentry_to_lower_path(dentry),
> +                                &lower_stat, request_mask, flags);
>         if (!rc) {
>                 fsstack_copy_attr_all(d_inode(dentry),
>                                       ecryptfs_inode_to_lower(d_inode(dentry)));
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 83ef66644c21..fca29dba7b14 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -171,7 +171,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>
>         type = ovl_path_real(dentry, &realpath);
>         old_cred = ovl_override_creds(dentry->d_sb);
> -       err = vfs_getattr(&realpath, stat, request_mask, flags);
> +       err = ovl_do_getattr(&realpath, stat, request_mask, flags);
>         if (err)
>                 goto out;
>
> @@ -196,8 +196,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>                                         (!is_dir ? STATX_NLINK : 0);
>
>                         ovl_path_lower(dentry, &realpath);
> -                       err = vfs_getattr(&realpath, &lowerstat,
> -                                         lowermask, flags);
> +                       err = ovl_do_getattr(&realpath, &lowerstat, lowermask,
> +                                            flags);
>                         if (err)
>                                 goto out;
>
> @@ -249,8 +249,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>
>                         ovl_path_lowerdata(dentry, &realpath);
>                         if (realpath.dentry) {
> -                               err = vfs_getattr(&realpath, &lowerdatastat,
> -                                                 lowermask, flags);
> +                               err = ovl_do_getattr(&realpath, &lowerdatastat,
> +                                                    lowermask, flags);
>                                 if (err)
>                                         goto out;
>                         } else {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 9817b2dcb132..09ca82ed0f8c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -397,6 +397,14 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
>         return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
>  }
>
> +static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
> +                                u32 request_mask, unsigned int flags)
> +{
> +       if (flags & AT_GETATTR_NOSEC)
> +               return vfs_getattr_nosec(path, stat, request_mask, flags);
> +       return vfs_getattr(path, stat, request_mask, flags);
> +}
> +
>  /* util.c */
>  int ovl_want_write(struct dentry *dentry);
>  void ovl_drop_write(struct dentry *dentry);
> diff --git a/fs/stat.c b/fs/stat.c
> index d43a5cc1bfa4..5375be5f97cc 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -133,7 +133,8 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>         idmap = mnt_idmap(path->mnt);
>         if (inode->i_op->getattr)
>                 return inode->i_op->getattr(idmap, path, stat,
> -                                           request_mask, query_flags);
> +                                           request_mask,
> +                                           query_flags | AT_GETATTR_NOSEC);
>
>         generic_fillattr(idmap, request_mask, inode, stat);
>         return 0;
> @@ -166,6 +167,9 @@ int vfs_getattr(const struct path *path, struct kstat *stat,
>  {
>         int retval;
>
> +       if (WARN_ON_ONCE(query_flags & AT_GETATTR_NOSEC))
> +               return -EPERM;
> +
>         retval = security_inode_getattr(path);
>         if (retval)
>                 return retval;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6c80f96049bd..282e90aeb163 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -116,5 +116,8 @@
>  #define AT_HANDLE_FID          AT_REMOVEDIR    /* file handle is needed to
>                                         compare object identity and may not
>                                         be usable to open_by_handle_at(2) */
> +#if defined(__KERNEL__)
> +#define AT_GETATTR_NOSEC       0x80000000
> +#endif
>
>  #endif /* _UAPI_LINUX_FCNTL_H */
> --
> 2.40.1
>

Stefan Berger Oct. 2, 2023, 1:52 p.m. UTC | #2

On 10/2/23 09:22, Amir Goldstein wrote:
> On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[e-mail]> wrote:
>> From: Stefan Berger <[e-mail]>
>>
>> When vfs_getattr_nosec() calls a filesystem's getattr interface function
>> then the 'nosec' should propagate into this function so that
>> vfs_getattr_nosec() can again be called from the filesystem's gettattr
>> rather than vfs_getattr(). The latter would add unnecessary security
>> checks that the initial vfs_getattr_nosec() call wanted to avoid.
>> Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
>> with the new getattr_flags parameter to the getattr interface function.
>> In overlayfs and ecryptfs use this flag to determine which one of the
>> two functions to call.
>>
>> In a recent code change introduced to IMA vfs_getattr_nosec() ended up
>> calling vfs_getattr() in overlayfs, which in turn called
>> security_inode_getattr() on an exiting process that did not have
>> current->fs set anymore, which then caused a kernel NULL pointer
>> dereference. With this change the call to security_inode_getattr() can
>> be avoided, thus avoiding the NULL pointer dereference.
>>
>> Reported-by: [e-mail]
>> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
>> Cc: Alexander Viro <[e-mail]>
>> Cc: [e-mail]
>> Cc: Miklos Szeredi <[e-mail]>
>> Cc: Amir Goldstein <[e-mail]>
>> Cc: Tyler Hicks <[e-mail]>
>> Cc: Mimi Zohar <[e-mail]>
>> Suggested-by: Christian Brauner <[e-mail]>
>> Co-developed-by: Amir Goldstein <[e-mail]>
>> Signed-off-by: Stefan Berger <[e-mail]>
>> ---
> Reviewed-by: Amir Goldstein <[e-mail]>
>
> Now let's see what vfs maintainers think about this...


Thanks.

For the other reviewers, the original syzbot message with the 'final 
oops' is here:

https://lore.kernel.org/linux-integrity/[e-mail]/T/#m616371f8ac1a316be13865e1699ac59ccf8ce6ef

Regards,

    Stefan


>
> Thanks,
> Amir.
>
>>   fs/ecryptfs/inode.c        | 12 ++++++++++--
>>   fs/overlayfs/inode.c       | 10 +++++-----
>>   fs/overlayfs/overlayfs.h   |  8 ++++++++
>>   fs/stat.c                  |  6 +++++-
>>   include/uapi/linux/fcntl.h |  3 +++
>>   5 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
>> index 992d9c7e64ae..5ab4b87888a7 100644
>> --- a/fs/ecryptfs/inode.c
>> +++ b/fs/ecryptfs/inode.c
>> @@ -998,6 +998,14 @@ static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
>>          return rc;
>>   }
>>
>> +static int ecryptfs_do_getattr(const struct path *path, struct kstat *stat,
>> +                              u32 request_mask, unsigned int flags)
>> +{
>> +       if (flags & AT_GETATTR_NOSEC)
>> +               return vfs_getattr_nosec(path, stat, request_mask, flags);
>> +       return vfs_getattr(path, stat, request_mask, flags);
>> +}
>> +
>>   static int ecryptfs_getattr(struct mnt_idmap *idmap,
>>                              const struct path *path, struct kstat *stat,
>>                              u32 request_mask, unsigned int flags)
>> @@ -1006,8 +1014,8 @@ static int ecryptfs_getattr(struct mnt_idmap *idmap,
>>          struct kstat lower_stat;
>>          int rc;
>>
>> -       rc = vfs_getattr(ecryptfs_dentry_to_lower_path(dentry), &lower_stat,
>> -                        request_mask, flags);
>> +       rc = ecryptfs_do_getattr(ecryptfs_dentry_to_lower_path(dentry),
>> +                                &lower_stat, request_mask, flags);
>>          if (!rc) {
>>                  fsstack_copy_attr_all(d_inode(dentry),
>>                                        ecryptfs_inode_to_lower(d_inode(dentry)));
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index 83ef66644c21..fca29dba7b14 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -171,7 +171,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>>
>>          type = ovl_path_real(dentry, &realpath);
>>          old_cred = ovl_override_creds(dentry->d_sb);
>> -       err = vfs_getattr(&realpath, stat, request_mask, flags);
>> +       err = ovl_do_getattr(&realpath, stat, request_mask, flags);
>>          if (err)
>>                  goto out;
>>
>> @@ -196,8 +196,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>>                                          (!is_dir ? STATX_NLINK : 0);
>>
>>                          ovl_path_lower(dentry, &realpath);
>> -                       err = vfs_getattr(&realpath, &lowerstat,
>> -                                         lowermask, flags);
>> +                       err = ovl_do_getattr(&realpath, &lowerstat, lowermask,
>> +                                            flags);
>>                          if (err)
>>                                  goto out;
>>
>> @@ -249,8 +249,8 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
>>
>>                          ovl_path_lowerdata(dentry, &realpath);
>>                          if (realpath.dentry) {
>> -                               err = vfs_getattr(&realpath, &lowerdatastat,
>> -                                                 lowermask, flags);
>> +                               err = ovl_do_getattr(&realpath, &lowerdatastat,
>> +                                                    lowermask, flags);
>>                                  if (err)
>>                                          goto out;
>>                          } else {
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 9817b2dcb132..09ca82ed0f8c 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -397,6 +397,14 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
>>          return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
>>   }
>>
>> +static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
>> +                                u32 request_mask, unsigned int flags)
>> +{
>> +       if (flags & AT_GETATTR_NOSEC)
>> +               return vfs_getattr_nosec(path, stat, request_mask, flags);
>> +       return vfs_getattr(path, stat, request_mask, flags);
>> +}
>> +
>>   /* util.c */
>>   int ovl_want_write(struct dentry *dentry);
>>   void ovl_drop_write(struct dentry *dentry);
>> diff --git a/fs/stat.c b/fs/stat.c
>> index d43a5cc1bfa4..5375be5f97cc 100644
>> --- a/fs/stat.c
>> +++ b/fs/stat.c
>> @@ -133,7 +133,8 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>>          idmap = mnt_idmap(path->mnt);
>>          if (inode->i_op->getattr)
>>                  return inode->i_op->getattr(idmap, path, stat,
>> -                                           request_mask, query_flags);
>> +                                           request_mask,
>> +                                           query_flags | AT_GETATTR_NOSEC);
>>
>>          generic_fillattr(idmap, request_mask, inode, stat);
>>          return 0;
>> @@ -166,6 +167,9 @@ int vfs_getattr(const struct path *path, struct kstat *stat,
>>   {
>>          int retval;
>>
>> +       if (WARN_ON_ONCE(query_flags & AT_GETATTR_NOSEC))
>> +               return -EPERM;
>> +
>>          retval = security_inode_getattr(path);
>>          if (retval)
>>                  return retval;
>> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
>> index 6c80f96049bd..282e90aeb163 100644
>> --- a/include/uapi/linux/fcntl.h
>> +++ b/include/uapi/linux/fcntl.h
>> @@ -116,5 +116,8 @@
>>   #define AT_HANDLE_FID          AT_REMOVEDIR    /* file handle is needed to
>>                                          compare object identity and may not
>>                                          be usable to open_by_handle_at(2) */
>> +#if defined(__KERNEL__)
>> +#define AT_GETATTR_NOSEC       0x80000000
>> +#endif
>>
>>   #endif /* _UAPI_LINUX_FCNTL_H */
>> --
>> 2.40.1
>>

Christian Brauner Oct. 3, 2023, 1:38 p.m. UTC | #3

On Mon, Oct 02, 2023 at 04:22:25PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[e-mail]> wrote:
> >
> > From: Stefan Berger <[e-mail]>
> >
> > When vfs_getattr_nosec() calls a filesystem's getattr interface function
> > then the 'nosec' should propagate into this function so that
> > vfs_getattr_nosec() can again be called from the filesystem's gettattr
> > rather than vfs_getattr(). The latter would add unnecessary security
> > checks that the initial vfs_getattr_nosec() call wanted to avoid.
> > Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
> > with the new getattr_flags parameter to the getattr interface function.
> > In overlayfs and ecryptfs use this flag to determine which one of the
> > two functions to call.
> >
> > In a recent code change introduced to IMA vfs_getattr_nosec() ended up
> > calling vfs_getattr() in overlayfs, which in turn called
> > security_inode_getattr() on an exiting process that did not have
> > current->fs set anymore, which then caused a kernel NULL pointer
> > dereference. With this change the call to security_inode_getattr() can
> > be avoided, thus avoiding the NULL pointer dereference.
> >
> > Reported-by: [e-mail]
> > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > Cc: Alexander Viro <[e-mail]>
> > Cc: [e-mail]
> > Cc: Miklos Szeredi <[e-mail]>
> > Cc: Amir Goldstein <[e-mail]>
> > Cc: Tyler Hicks <[e-mail]>
> > Cc: Mimi Zohar <[e-mail]>
> > Suggested-by: Christian Brauner <[e-mail]>
> > Co-developed-by: Amir Goldstein <[e-mail]>
> > Signed-off-by: Stefan Berger <[e-mail]>
> > ---
> 
> Reviewed-by: Amir Goldstein <[e-mail]>
> 
> Now let's see what vfs maintainers think about this...

Seems fine overall. We kind of need to propagate the knowledge through
the layers. But I don't like that we need something like it...

Mimi Zohar Oct. 10, 2023, 1:14 a.m. UTC | #4

On Tue, 2023-10-03 at 15:38 +0200, Christian Brauner wrote:
> On Mon, Oct 02, 2023 at 04:22:25PM +0300, Amir Goldstein wrote:
> > On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[e-mail]> wrote:
> > >
> > > From: Stefan Berger <[e-mail]>
> > >
> > > When vfs_getattr_nosec() calls a filesystem's getattr interface function
> > > then the 'nosec' should propagate into this function so that
> > > vfs_getattr_nosec() can again be called from the filesystem's gettattr
> > > rather than vfs_getattr(). The latter would add unnecessary security
> > > checks that the initial vfs_getattr_nosec() call wanted to avoid.
> > > Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
> > > with the new getattr_flags parameter to the getattr interface function.
> > > In overlayfs and ecryptfs use this flag to determine which one of the
> > > two functions to call.
> > >
> > > In a recent code change introduced to IMA vfs_getattr_nosec() ended up
> > > calling vfs_getattr() in overlayfs, which in turn called
> > > security_inode_getattr() on an exiting process that did not have
> > > current->fs set anymore, which then caused a kernel NULL pointer
> > > dereference. With this change the call to security_inode_getattr() can
> > > be avoided, thus avoiding the NULL pointer dereference.
> > >
> > > Reported-by: [e-mail]
> > > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > > Cc: Alexander Viro <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Miklos Szeredi <[e-mail]>
> > > Cc: Amir Goldstein <[e-mail]>
> > > Cc: Tyler Hicks <[e-mail]>
> > > Cc: Mimi Zohar <[e-mail]>
> > > Suggested-by: Christian Brauner <[e-mail]>
> > > Co-developed-by: Amir Goldstein <[e-mail]>
> > > Signed-off-by: Stefan Berger <[e-mail]>
> > > ---
> > 
> > Reviewed-by: Amir Goldstein <[e-mail]>
> > 
> > Now let's see what vfs maintainers think about this...
> 
> Seems fine overall. We kind of need to propagate the knowledge through
> the layers. But I don't like that we need something like it...

So at this point there are two options.  Either revert commit
db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") or
this patch to fix it.  Christian, what do you prefer?

Mimi

Christian Brauner Oct. 10, 2023, 8:35 a.m. UTC | #5

On Mon, 02 Oct 2023 08:57:33 -0400, Stefan Berger wrote:
> When vfs_getattr_nosec() calls a filesystem's getattr interface function
> then the 'nosec' should propagate into this function so that
> vfs_getattr_nosec() can again be called from the filesystem's gettattr
> rather than vfs_getattr(). The latter would add unnecessary security
> checks that the initial vfs_getattr_nosec() call wanted to avoid.
> Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
> with the new getattr_flags parameter to the getattr interface function.
> In overlayfs and ecryptfs use this flag to determine which one of the
> two functions to call.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: Pass AT_GETATTR_NOSEC flag to getattr interface function
      https://git.kernel.org/vfs/vfs/c/6ea042691c74

Stefan Berger Nov. 6, 2023, 7:44 p.m. UTC | #6

On 10/10/23 04:35, Christian Brauner wrote:
> On Mon, 02 Oct 2023 08:57:33 -0400, Stefan Berger wrote:
>> When vfs_getattr_nosec() calls a filesystem's getattr interface function
>> then the 'nosec' should propagate into this function so that
>> vfs_getattr_nosec() can again be called from the filesystem's gettattr
>> rather than vfs_getattr(). The latter would add unnecessary security
>> checks that the initial vfs_getattr_nosec() call wanted to avoid.
>> Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
>> with the new getattr_flags parameter to the getattr interface function.
>> In overlayfs and ecryptfs use this flag to determine which one of the
>> two functions to call.
>>
>> [...]
> 
> Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> Patches in the vfs.fixes branch should appear in linux-next soon.

Did something happen to this patch? I do not see it in your branch nor 
the linux-next one nor Linus's tree.



> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.fixes
> 
> [1/1] fs: Pass AT_GETATTR_NOSEC flag to getattr interface function
>        https://git.kernel.org/vfs/vfs/c/6ea042691c74

Christian Brauner Nov. 8, 2023, 9:20 a.m. UTC | #7

> Did something happen to this patch? I do not see it in your branch nor the
> linux-next one nor Linus's tree.

Sorry, my bad. I'll send that out as a fixes pr soon.

J. R. Okajima Dec. 7, 2023, 9:10 p.m. UTC | #8

Stefan Berger:
> When vfs_getattr_nosec() calls a filesystem's getattr interface function
> then the 'nosec' should propagate into this function so that
> vfs_getattr_nosec() can again be called from the filesystem's gettattr
> rather than vfs_getattr(). The latter would add unnecessary security
> checks that the initial vfs_getattr_nosec() call wanted to avoid.
> Therefore, introduce the getattr flag GETATTR_NOSEC and allow to pass
> with the new getattr_flags parameter to the getattr interface function.
> In overlayfs and ecryptfs use this flag to determine which one of the
> two functions to call.

You are introducing two perfectly identical functions.
ecryptfs_do_getattr() and ovl_do_getattr().
Why don't you provide one in a common place, such like
include/linux/fs_stack.h?


J. R. Okajima

Patch

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 992d9c7e64ae..5ab4b87888a7 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -998,6 +998,14 @@  static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
 	return rc;
 }
 
+static int ecryptfs_do_getattr(const struct path *path, struct kstat *stat,
+			       u32 request_mask, unsigned int flags)
+{
+	if (flags & AT_GETATTR_NOSEC)
+		return vfs_getattr_nosec(path, stat, request_mask, flags);
+	return vfs_getattr(path, stat, request_mask, flags);
+}
+
 static int ecryptfs_getattr(struct mnt_idmap *idmap,
 			    const struct path *path, struct kstat *stat,
 			    u32 request_mask, unsigned int flags)
@@ -1006,8 +1014,8 @@  static int ecryptfs_getattr(struct mnt_idmap *idmap,
 	struct kstat lower_stat;
 	int rc;
 
-	rc = vfs_getattr(ecryptfs_dentry_to_lower_path(dentry), &lower_stat,
-			 request_mask, flags);
+	rc = ecryptfs_do_getattr(ecryptfs_dentry_to_lower_path(dentry),
+				 &lower_stat, request_mask, flags);
 	if (!rc) {
 		fsstack_copy_attr_all(d_inode(dentry),
 				      ecryptfs_inode_to_lower(d_inode(dentry)));
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 83ef66644c21..fca29dba7b14 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -171,7 +171,7 @@  int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 
 	type = ovl_path_real(dentry, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
-	err = vfs_getattr(&realpath, stat, request_mask, flags);
+	err = ovl_do_getattr(&realpath, stat, request_mask, flags);
 	if (err)
 		goto out;
 
@@ -196,8 +196,8 @@  int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 					(!is_dir ? STATX_NLINK : 0);
 
 			ovl_path_lower(dentry, &realpath);
-			err = vfs_getattr(&realpath, &lowerstat,
-					  lowermask, flags);
+			err = ovl_do_getattr(&realpath, &lowerstat, lowermask,
+					     flags);
 			if (err)
 				goto out;
 
@@ -249,8 +249,8 @@  int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 
 			ovl_path_lowerdata(dentry, &realpath);
 			if (realpath.dentry) {
-				err = vfs_getattr(&realpath, &lowerdatastat,
-						  lowermask, flags);
+				err = ovl_do_getattr(&realpath, &lowerdatastat,
+						     lowermask, flags);
 				if (err)
 					goto out;
 			} else {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9817b2dcb132..09ca82ed0f8c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -397,6 +397,14 @@  static inline bool ovl_open_flags_need_copy_up(int flags)
 	return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
 }
 
+static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
+				 u32 request_mask, unsigned int flags)
+{
+	if (flags & AT_GETATTR_NOSEC)
+		return vfs_getattr_nosec(path, stat, request_mask, flags);
+	return vfs_getattr(path, stat, request_mask, flags);
+}
+
 /* util.c */
 int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
diff --git a/fs/stat.c b/fs/stat.c
index d43a5cc1bfa4..5375be5f97cc 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -133,7 +133,8 @@  int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 	idmap = mnt_idmap(path->mnt);
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(idmap, path, stat,
-					    request_mask, query_flags);
+					    request_mask,
+					    query_flags | AT_GETATTR_NOSEC);
 
 	generic_fillattr(idmap, request_mask, inode, stat);
 	return 0;
@@ -166,6 +167,9 @@  int vfs_getattr(const struct path *path, struct kstat *stat,
 {
 	int retval;
 
+	if (WARN_ON_ONCE(query_flags & AT_GETATTR_NOSEC))
+		return -EPERM;
+
 	retval = security_inode_getattr(path);
 	if (retval)
 		return retval;
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6c80f96049bd..282e90aeb163 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -116,5 +116,8 @@ 
 #define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
 					compare object identity and may not
 					be usable to open_by_handle_at(2) */
+#if defined(__KERNEL__)
+#define AT_GETATTR_NOSEC	0x80000000
+#endif
 
 #endif /* _UAPI_LINUX_FCNTL_H */