[X/B][SRU][CVE-2018-20976][PATCH] xfs: clear sb->s_fs_info on mount failure

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[X/B][SRU][CVE-2018-20976][PATCH] xfs: clear sb->s_fs_info on mount failure

Connor Kuehl
From: Dave Chinner <[hidden email]>

CVE-2018-20976

We recently had an oops reported on a 4.14 kernel in
xfs_reclaim_inodes_count() where sb->s_fs_info pointed to garbage
and so the m_perag_tree lookup walked into lala land.

Essentially, the machine was under memory pressure when the mount
was being run, xfs_fs_fill_super() failed after allocating the
xfs_mount and attaching it to sb->s_fs_info. It then cleaned up and
freed the xfs_mount, but the sb->s_fs_info field still pointed to
the freed memory. Hence when the superblock shrinker then ran
it fell off the bad pointer.

With the superblock shrinker problem fixed at teh VFS level, this
stale s_fs_info pointer is still a problem - we use it
unconditionally in ->put_super when the superblock is being torn
down, and hence we can still trip over it after a ->fill_super
call failure. Hence we need to clear s_fs_info if
xfs-fs_fill_super() fails, and we need to check if it's valid in
the places it can potentially be dereferenced after a ->fill_super
failure.

Signed-Off-By: Dave Chinner <[hidden email]>
Reviewed-by: Darrick J. Wong <[hidden email]>
Signed-off-by: Darrick J. Wong <[hidden email]>
(cherry picked from commit c9fbd7bbc23dbdd73364be4d045e5d3612cf6e82)
Signed-off-by: Connor Kuehl <[hidden email]>
---
 fs/xfs/xfs_super.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ef64a1e1a66a..ff3f5812c0fd 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1572,6 +1572,7 @@ xfs_fs_fill_super(
  out_close_devices:
  xfs_close_devices(mp);
  out_free_fsname:
+ sb->s_fs_info = NULL;
  xfs_free_fsname(mp);
  kfree(mp);
  out:
@@ -1589,6 +1590,10 @@ xfs_fs_put_super(
 {
  struct xfs_mount *mp = XFS_M(sb);
 
+ /* if ->fill_super failed, we have no mount to tear down */
+ if (!sb->s_fs_info)
+ return;
+
  xfs_notice(mp, "Unmounting Filesystem");
  xfs_filestream_unmount(mp);
  xfs_unmountfs(mp);
@@ -1598,6 +1603,8 @@ xfs_fs_put_super(
  xfs_destroy_percpu_counters(mp);
  xfs_destroy_mount_workqueues(mp);
  xfs_close_devices(mp);
+
+ sb->s_fs_info = NULL;
  xfs_free_fsname(mp);
  kfree(mp);
 }
@@ -1617,6 +1624,9 @@ xfs_fs_nr_cached_objects(
  struct super_block *sb,
  struct shrink_control *sc)
 {
+ /* Paranoia: catch incorrect calls during mount setup or teardown */
+ if (WARN_ON_ONCE(!sb->s_fs_info))
+ return 0;
  return xfs_reclaim_inodes_count(XFS_M(sb));
 }
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [X/B][SRU][CVE-2018-20976][PATCH] xfs: clear sb->s_fs_info on mount failure

Kleber Souza
On 8/29/19 4:33 PM, Connor Kuehl wrote:

> From: Dave Chinner <[hidden email]>
>
> CVE-2018-20976
>
> We recently had an oops reported on a 4.14 kernel in
> xfs_reclaim_inodes_count() where sb->s_fs_info pointed to garbage
> and so the m_perag_tree lookup walked into lala land.
>
> Essentially, the machine was under memory pressure when the mount
> was being run, xfs_fs_fill_super() failed after allocating the
> xfs_mount and attaching it to sb->s_fs_info. It then cleaned up and
> freed the xfs_mount, but the sb->s_fs_info field still pointed to
> the freed memory. Hence when the superblock shrinker then ran
> it fell off the bad pointer.
>
> With the superblock shrinker problem fixed at teh VFS level, this
> stale s_fs_info pointer is still a problem - we use it
> unconditionally in ->put_super when the superblock is being torn
> down, and hence we can still trip over it after a ->fill_super
> call failure. Hence we need to clear s_fs_info if
> xfs-fs_fill_super() fails, and we need to check if it's valid in
> the places it can potentially be dereferenced after a ->fill_super
> failure.
>
> Signed-Off-By: Dave Chinner <[hidden email]>
> Reviewed-by: Darrick J. Wong <[hidden email]>
> Signed-off-by: Darrick J. Wong <[hidden email]>
> (cherry picked from commit c9fbd7bbc23dbdd73364be4d045e5d3612cf6e82)
> Signed-off-by: Connor Kuehl <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  fs/xfs/xfs_super.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ef64a1e1a66a..ff3f5812c0fd 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1572,6 +1572,7 @@ xfs_fs_fill_super(
>   out_close_devices:
>   xfs_close_devices(mp);
>   out_free_fsname:
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>   out:
> @@ -1589,6 +1590,10 @@ xfs_fs_put_super(
>  {
>   struct xfs_mount *mp = XFS_M(sb);
>  
> + /* if ->fill_super failed, we have no mount to tear down */
> + if (!sb->s_fs_info)
> + return;
> +
>   xfs_notice(mp, "Unmounting Filesystem");
>   xfs_filestream_unmount(mp);
>   xfs_unmountfs(mp);
> @@ -1598,6 +1603,8 @@ xfs_fs_put_super(
>   xfs_destroy_percpu_counters(mp);
>   xfs_destroy_mount_workqueues(mp);
>   xfs_close_devices(mp);
> +
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>  }
> @@ -1617,6 +1624,9 @@ xfs_fs_nr_cached_objects(
>   struct super_block *sb,
>   struct shrink_control *sc)
>  {
> + /* Paranoia: catch incorrect calls during mount setup or teardown */
> + if (WARN_ON_ONCE(!sb->s_fs_info))
> + return 0;
>   return xfs_reclaim_inodes_count(XFS_M(sb));
>  }
>  
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [X/B][SRU][CVE-2018-20976][PATCH] xfs: clear sb->s_fs_info on mount failure

Khaled Elmously
In reply to this post by Connor Kuehl
On 2019-08-29 07:33:41 , Connor Kuehl wrote:

> From: Dave Chinner <[hidden email]>
>
> CVE-2018-20976
>
> We recently had an oops reported on a 4.14 kernel in
> xfs_reclaim_inodes_count() where sb->s_fs_info pointed to garbage
> and so the m_perag_tree lookup walked into lala land.
>
> Essentially, the machine was under memory pressure when the mount
> was being run, xfs_fs_fill_super() failed after allocating the
> xfs_mount and attaching it to sb->s_fs_info. It then cleaned up and
> freed the xfs_mount, but the sb->s_fs_info field still pointed to
> the freed memory. Hence when the superblock shrinker then ran
> it fell off the bad pointer.
>
> With the superblock shrinker problem fixed at teh VFS level, this
> stale s_fs_info pointer is still a problem - we use it
> unconditionally in ->put_super when the superblock is being torn
> down, and hence we can still trip over it after a ->fill_super
> call failure. Hence we need to clear s_fs_info if
> xfs-fs_fill_super() fails, and we need to check if it's valid in
> the places it can potentially be dereferenced after a ->fill_super
> failure.
>
> Signed-Off-By: Dave Chinner <[hidden email]>
> Reviewed-by: Darrick J. Wong <[hidden email]>
> Signed-off-by: Darrick J. Wong <[hidden email]>
> (cherry picked from commit c9fbd7bbc23dbdd73364be4d045e5d3612cf6e82)
> Signed-off-by: Connor Kuehl <[hidden email]>
> ---
>  fs/xfs/xfs_super.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ef64a1e1a66a..ff3f5812c0fd 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1572,6 +1572,7 @@ xfs_fs_fill_super(
>   out_close_devices:
>   xfs_close_devices(mp);
>   out_free_fsname:
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>   out:
> @@ -1589,6 +1590,10 @@ xfs_fs_put_super(
>  {
>   struct xfs_mount *mp = XFS_M(sb);
>  
> + /* if ->fill_super failed, we have no mount to tear down */
> + if (!sb->s_fs_info)
> + return;
> +
>   xfs_notice(mp, "Unmounting Filesystem");
>   xfs_filestream_unmount(mp);
>   xfs_unmountfs(mp);
> @@ -1598,6 +1603,8 @@ xfs_fs_put_super(
>   xfs_destroy_percpu_counters(mp);
>   xfs_destroy_mount_workqueues(mp);
>   xfs_close_devices(mp);
> +
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>  }
> @@ -1617,6 +1624,9 @@ xfs_fs_nr_cached_objects(
>   struct super_block *sb,
>   struct shrink_control *sc)
>  {
> + /* Paranoia: catch incorrect calls during mount setup or teardown */
> + if (WARN_ON_ONCE(!sb->s_fs_info))
> + return 0;
>   return xfs_reclaim_inodes_count(XFS_M(sb));
>  }
>  
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [X/B][SRU][CVE-2018-20976][PATCH] xfs: clear sb->s_fs_info on mount failure

Khaled Elmously
In reply to this post by Connor Kuehl
On 2019-08-29 07:33:41 , Connor Kuehl wrote:

> From: Dave Chinner <[hidden email]>
>
> CVE-2018-20976
>
> We recently had an oops reported on a 4.14 kernel in
> xfs_reclaim_inodes_count() where sb->s_fs_info pointed to garbage
> and so the m_perag_tree lookup walked into lala land.
>
> Essentially, the machine was under memory pressure when the mount
> was being run, xfs_fs_fill_super() failed after allocating the
> xfs_mount and attaching it to sb->s_fs_info. It then cleaned up and
> freed the xfs_mount, but the sb->s_fs_info field still pointed to
> the freed memory. Hence when the superblock shrinker then ran
> it fell off the bad pointer.
>
> With the superblock shrinker problem fixed at teh VFS level, this
> stale s_fs_info pointer is still a problem - we use it
> unconditionally in ->put_super when the superblock is being torn
> down, and hence we can still trip over it after a ->fill_super
> call failure. Hence we need to clear s_fs_info if
> xfs-fs_fill_super() fails, and we need to check if it's valid in
> the places it can potentially be dereferenced after a ->fill_super
> failure.
>
> Signed-Off-By: Dave Chinner <[hidden email]>
> Reviewed-by: Darrick J. Wong <[hidden email]>
> Signed-off-by: Darrick J. Wong <[hidden email]>
> (cherry picked from commit c9fbd7bbc23dbdd73364be4d045e5d3612cf6e82)
> Signed-off-by: Connor Kuehl <[hidden email]>
> ---
>  fs/xfs/xfs_super.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ef64a1e1a66a..ff3f5812c0fd 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1572,6 +1572,7 @@ xfs_fs_fill_super(
>   out_close_devices:
>   xfs_close_devices(mp);
>   out_free_fsname:
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>   out:
> @@ -1589,6 +1590,10 @@ xfs_fs_put_super(
>  {
>   struct xfs_mount *mp = XFS_M(sb);
>  
> + /* if ->fill_super failed, we have no mount to tear down */
> + if (!sb->s_fs_info)
> + return;
> +
>   xfs_notice(mp, "Unmounting Filesystem");
>   xfs_filestream_unmount(mp);
>   xfs_unmountfs(mp);
> @@ -1598,6 +1603,8 @@ xfs_fs_put_super(
>   xfs_destroy_percpu_counters(mp);
>   xfs_destroy_mount_workqueues(mp);
>   xfs_close_devices(mp);
> +
> + sb->s_fs_info = NULL;
>   xfs_free_fsname(mp);
>   kfree(mp);
>  }
> @@ -1617,6 +1624,9 @@ xfs_fs_nr_cached_objects(
>   struct super_block *sb,
>   struct shrink_control *sc)
>  {
> + /* Paranoia: catch incorrect calls during mount setup or teardown */
> + if (WARN_ON_ONCE(!sb->s_fs_info))
> + return 0;
>   return xfs_reclaim_inodes_count(XFS_M(sb));
>  }
>  
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team