On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[email protected]> wrote:
>
> From: Stefan Berger <[email protected]>
>
> 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: [email protected]
> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> Cc: Miklos Szeredi <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Tyler Hicks <[email protected]>
> Cc: Mimi Zohar <[email protected]>
> Suggested-by: Christian Brauner <[email protected]>
> Co-developed-by: Amir Goldstein <[email protected]>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
Reviewed-by: Amir Goldstein <[email protected]>
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
>
On 10/2/23 09:22, Amir Goldstein wrote:
> On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[email protected]> wrote:
>> From: Stefan Berger <[email protected]>
>>
>> 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: [email protected]
>> Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
>> Cc: Alexander Viro <[email protected]>
>> Cc: [email protected]
>> Cc: Miklos Szeredi <[email protected]>
>> Cc: Amir Goldstein <[email protected]>
>> Cc: Tyler Hicks <[email protected]>
>> Cc: Mimi Zohar <[email protected]>
>> Suggested-by: Christian Brauner <[email protected]>
>> Co-developed-by: Amir Goldstein <[email protected]>
>> Signed-off-by: Stefan Berger <[email protected]>
>> ---
> Reviewed-by: Amir Goldstein <[email protected]>
>
> 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/[email protected]/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
>>
On Mon, Oct 02, 2023 at 04:22:25PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2023 at 3:57 PM Stefan Berger <[email protected]> wrote:
> >
> > From: Stefan Berger <[email protected]>
> >
> > 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: [email protected]
> > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > Cc: Alexander Viro <[email protected]>
> > Cc: [email protected]
> > Cc: Miklos Szeredi <[email protected]>
> > Cc: Amir Goldstein <[email protected]>
> > Cc: Tyler Hicks <[email protected]>
> > Cc: Mimi Zohar <[email protected]>
> > Suggested-by: Christian Brauner <[email protected]>
> > Co-developed-by: Amir Goldstein <[email protected]>
> > Signed-off-by: Stefan Berger <[email protected]>
> > ---
>
> Reviewed-by: Amir Goldstein <[email protected]>
>
> 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...
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 <[email protected]> wrote:
> > >
> > > From: Stefan Berger <[email protected]>
> > >
> > > 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: [email protected]
> > > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > > Cc: Alexander Viro <[email protected]>
> > > Cc: [email protected]
> > > Cc: Miklos Szeredi <[email protected]>
> > > Cc: Amir Goldstein <[email protected]>
> > > Cc: Tyler Hicks <[email protected]>
> > > Cc: Mimi Zohar <[email protected]>
> > > Suggested-by: Christian Brauner <[email protected]>
> > > Co-developed-by: Amir Goldstein <[email protected]>
> > > Signed-off-by: Stefan Berger <[email protected]>
> > > ---
> >
> > Reviewed-by: Amir Goldstein <[email protected]>
> >
> > 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