Re: [PATCH] fs: Pass AT_GETATTR_NOSEC flag to getattr interface function

[Stefan Berger <stefanb@linux.ibm.com>]

On 10/2/23 09:22, Amir Goldstein wrote:
> On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> 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: syzbot+a67fc5321ffb4b311c98@syzkaller.appspotmail.com
>> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: linux-fsdevel@vger.kernel.org
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Cc: Amir Goldstein <amir73il@gmail.com>
>> Cc: Tyler Hicks <code@tyhicks.com>
>> Cc: Mimi Zohar <zohar@linux.ibm.com>
>> Suggested-by: Christian Brauner <brauner@kernel.org>
>> Co-developed-by: Amir Goldstein <amir73il@gmail.com>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>
> 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/bed99e92-cb7c-868d-94f3-ddf53e2b262a@linux.ibm.com/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
>>