linux-ext4 - Re: [BUG] invalid superblock checksum possibly due to race

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 30 Jun 2020 13:48:32 +0200
From:   Jan Kara <jack@...e.cz>
To:     Costa Sapuntzakis <costa@...estorage.com>
Cc:     linux-ext4@...r.kernel.org
Subject: Re: [BUG] invalid superblock checksum possibly due to race

Hello!

On Wed 24-06-20 16:56:18, Costa Sapuntzakis wrote:
> Our workload: taking snapshots repeatedly of an active ext4 filesystem
> (vdbench fwiw). e2fsck discovered a snapshot that had a corrupted
> superblock after journal replay. Diffing the corrupted superblock to
> the superblock before journal replay revealed that only s_last_orphan
> and the checksum had changed.
> 
> The following race could explain it:
> 
> Thread 1 (T1): ext4_orphan_del -> update s_last_orphan to value A ->
> ext4_handle_dirty_super -> ext4_superblock_csum_set -- PAUSE right
> before setting es->s_checksum
> 
> T2: ext4_orphan_del -> update s_last_orphan to value B ->
> ext4_handle_dirty_super -> ext4_superblock_csum_set runs to completion
> 
> T1: Resume and assign es->s_checksum
> 
> Is there higher level synchronization going on that makes this race benign?

Thanks for report and the analysis. What you describe indeed seems
possible.

> If not, a spinlock around the calculation and assignment should fix it.

Yes, probably ext4_superblock_csum_set() should use

lock_buffer(EXT4_SB(sb)->s_sbh)

to synchronize updating of superblock checksum. Will you send a patch?

> The spinlock still has the race where s_last_orphan is being updated
> while the checksum is calculated. But the last thread to set
> s_last_orphan will also eventually try to recalculate the checksum and
> set it right (though it's possible some other thread will do it for
> it). And I'm guessing/hoping jbd2 won't flush the superblock to the
> journal and close a transaction until the references from
> journal_get_write_access drain. The checksum is recalculated before
> the get_write_access reference is dropped.

Yes, jbd2 layer will make sure that inconsistent block contents will not
make it to disk in this case.

								Honza

-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR