linux-ext4 - Re: [PATCH 3/3] ext4: fix potential race between s_flex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 21 Feb 2020 19:53:37 +0000
From:   "Jitindar SIngh, Suraj" <surajjs@...zon.com>
To:     "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "tytso@....edu" <tytso@....edu>
CC:     "cai@....pw" <cai@....pw>, "stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH 3/3] ext4: fix potential race between s_flex_groups online
 resizing and access

On Fri, 2020-02-21 at 00:34 -0500, Theodore Ts'o wrote:
> From: Suraj Jitindar Singh <surajjs@...zon.com>
> 
> During an online resize an array of s_flex_groups structures gets
> replaced
> so it can get enlarged. If there is a concurrent access to the array
> and
> this memory has been reused then this can lead to an invalid memory
> access.
> 
> The s_flex_group array has been converted into an array of pointers
> rather
> than an array of structures. This is to ensure that the information
> contained in the structures cannot get out of sync during a resize
> due to
> an accessor updating the value in the old structure after it has been
> copied but before the array pointer is updated. Since the structures
> them-
> selves are no longer copied but only the pointers to them this case
> is
> mitigated.

A bug with this patch that I missed has been pointed out on the mailing
list:
https://lore.kernel.org/linux-ext4/1582293736.7365.109.camel@lca.pw

> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206443
> Previous-Patch-Link: 
> https://lore.kernel.org/r/20200219030851.2678-4-surajjs@amazon.com
> Signed-off-by: Suraj Jitindar Singh <surajjs@...zon.com>
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> Cc: stable@...nel.org
> ---
>  fs/ext4/ext4.h    |  2 +-
>  fs/ext4/ialloc.c  | 23 +++++++++------
>  fs/ext4/mballoc.c |  9 ++++--
>  fs/ext4/resize.c  |  7 +++--
>  fs/ext4/super.c   | 72 ++++++++++++++++++++++++++++++++-------------
> --
>  5 files changed, 76 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b1ece5329738..614fefa7dc7a 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1512,7 +1512,7 @@ struct ext4_sb_info {
>  	unsigned int s_extent_max_zeroout_kb;
>  
>  	unsigned int s_log_groups_per_flex;
> -	struct flex_groups *s_flex_groups;
> +	struct flex_groups * __rcu *s_flex_groups;
>  	ext4_group_t s_flex_groups_allocated;
>  
>  	/* workqueue for reserved extent conversions (buffered io) */
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index c66e8f9451a2..501118b9ba90 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -328,11 +328,13 @@ void ext4_free_inode(handle_t *handle, struct
> inode *inode)
>  
>  	percpu_counter_inc(&sbi->s_freeinodes_counter);
>  	if (sbi->s_log_groups_per_flex) {
> -		ext4_group_t f = ext4_flex_group(sbi, block_group);
> +		struct flex_groups *fg;
>  
> -		atomic_inc(&sbi->s_flex_groups[f].free_inodes);
> +		fg = sbi_array_rcu_deref(sbi, s_flex_groups,
> +					 ext4_flex_group(sbi,
> block_group));
> +		atomic_inc(&fg->free_inodes);
>  		if (is_directory)
> -			atomic_dec(&sbi->s_flex_groups[f].used_dirs);
> +			atomic_dec(&fg->used_dirs);
>  	}
>  	BUFFER_TRACE(bh2, "call ext4_handle_dirty_metadata");
>  	fatal = ext4_handle_dirty_metadata(handle, NULL, bh2);
> @@ -368,12 +370,13 @@ static void get_orlov_stats(struct super_block
> *sb, ext4_group_t g,
>  			    int flex_size, struct orlov_stats *stats)
>  {
>  	struct ext4_group_desc *desc;
> -	struct flex_groups *flex_group = EXT4_SB(sb)->s_flex_groups;
> +	struct flex_groups *flex_group =
> sbi_array_rcu_deref(EXT4_SB(sb),
> +							     s_flex_gro
> ups, g);

The assignment to flex_group needs to happen within the
if (flex_size > 1) {}
if statement to avoid a potential null pointer dereference.

if (flex_size > 1) {
	struct flex_groups *flex_group =
sbi_array_rcu_deref(EXT4_SB(sb), s_flex_groups, g);
	...
}

Would you like a resend?

>  
>  	if (flex_size > 1) {
> -		stats->free_inodes =
> atomic_read(&flex_group[g].free_inodes);
> -		stats->free_clusters =
> atomic64_read(&flex_group[g].free_clusters);
> -		stats->used_dirs =
> atomic_read(&flex_group[g].used_dirs);
> +		stats->free_inodes = atomic_read(&flex_group-
> >free_inodes);
> +		stats->free_clusters = atomic64_read(&flex_group-
> >free_clusters);
> +		stats->used_dirs = atomic_read(&flex_group->used_dirs);
>  		return;
>  	}
>  
[snip]