hfs_mdb_commit() and hfs_mdb_close() keep the MDB and alternate-MDB
buffer heads (mdb_bh, alt_mdb_bh) pinned for the lifetime of the
mount and write into them in place. If a write against the backing
device fails, the block layer clears BH_Uptodate on the buffer
(end_buffer_write_sync(), fs/buffer.c) to mark its contents as no
longer trustworthy.
!buffer_uptodate(bh)
WARNING: fs/buffer.c:1093 at mark_buffer_dirty+0x299/0x410 fs/buffer.c:1093, CPU#0: syz.1.1033/6784
Modules linked in:
CPU: 0 UID: 0 PID: 6784 Comm: syz.1.1033 Not tainted syzkaller #0 PREEMPT(full)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
RIP: 0010:mark_buffer_dirty+0x299/0x410 fs/buffer.c:1093
Code: 4c 89 f7 e8 f9 0d d9 ff 49 8b 3e be 40 00 00 00 5b 41 5c 41 5e 41 5f 5d e9 d4 5e fb ff e8 af e7 69 ff eb 8c e8 a8 e7 69 ff 90 <0f> 0b 90 e9 a5 fd ff ff e8 9a e7 69 ff 90 0f 0b 90 e9 cf fd ff ff
RSP: 0018:ffffc90005d379a8 EFLAGS: 00010293
RAX: ffffffff825c9ae8 RBX: ffff8880123aa700 RCX: ffff888034a30000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000002
RBP: 1ffff1100a45f401 R08: ffff8880123aa707 R09: 1ffff110024754e0
R10: dffffc0000000000 R11: ffffed10024754e1 R12: dffffc0000000000
R13: ffff8880522fa668 R14: ffff888045ba945b R15: ffff888045ba9400
FS: 000055556b366500(0000) GS:ffff88808c54e000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b31a63fff CR3: 0000000034b1d000 CR4: 0000000000352ef0
Call Trace:
hfs_mdb_commit+0x503/0x1160 fs/hfs/mdb.c:309
hfs_sync_fs+0x1d/0x30 fs/hfs/super.c:38
sync_filesystem+0x1cf/0x230 fs/sync.c:66
fs_bdev_sync+0x2c/0x50 fs/super.c:1433
blkdev_flushbuf block/ioctl.c:520 [inline]
blkdev_common_ioctl+0x12e1/0x3250 block/ioctl.c:658
blkdev_ioctl+0x528/0x740 block/ioctl.c:791
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f1d9239de59
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffd0ece148 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f1d92625fa0 RCX: 00007f1d9239de59
RDX: 0000000000000000 RSI: 0000000000001261 RDI: 0000000000000004
RBP: 00007f1d92433e6f R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007f1d92625fac R14: 00007f1d92625fa0 R15: 00007f1d92625fa0
This was found by fuzzing an HFS image on a loop device backed by a
memfd; under memory pressure the shmem write-back path returned
-ENOSPC (mm/shmem.c, shmem_acct_blocks()), which surfaced as a plain
write error on the block device. The call path: ioctl(BLKFLSBUF) ->
blkdev_common_ioctl() -> sync_filesystem() -> hfs_sync_fs() ->
hfs_mdb_commit().
Check buffer_uptodate() on mdb_bh/alt_mdb_bh before dirtying them.
If either has gone stale, mark the volume read-only and skip the
write instead of asserting on it. Also, hfs_mdb_commit() returns
-EIO error code in such situation. As a result, hfs_sync_fs() can
report the failure instead of always returning 0.
The sync()/fsync()/BLKFLSBUF now observe -EIO instead of silently
succeeding while the volume goes read-only underneath them.
Reported-by: syzbot+c149ad75e9633be0c1ad@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c149ad75e9633be0c1ad
Signed-off-by: Viacheslav Dubeyko
cc: John Paul Adrian Glaubitz
cc: Yangtao Li
cc: linux-fsdevel@vger.kernel.org
---
fs/hfs/hfs_fs.h | 2 +-
fs/hfs/mdb.c | 28 +++++++++++++++++++++++++---
fs/hfs/super.c | 3 +--
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index f3624514fcb0..8e9a1673a142 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -201,7 +201,7 @@ extern const struct xattr_handler * const hfs_xattr_handlers[];
/* mdb.c */
extern bool is_hfs_cnid_counts_valid(struct super_block *sb);
extern int hfs_mdb_get(struct super_block *sb);
-extern void hfs_mdb_commit(struct super_block *sb);
+extern int hfs_mdb_commit(struct super_block *sb);
extern void hfs_mdb_close(struct super_block *sb);
extern void hfs_mdb_put(struct super_block *sb);
diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
index a97cea35ca2e..f4578f634302 100644
--- a/fs/hfs/mdb.c
+++ b/fs/hfs/mdb.c
@@ -274,7 +274,9 @@ int hfs_mdb_get(struct super_block *sb)
* Output Variable(s):
* NONE
* Returns:
- * void
+ * 0 on success, -EIO if the MDB or alternate MDB buffer is no longer
+ * valid (e.g. after a prior write error), in which case the volume is
+ * remounted read-only.
* Preconditions:
* 'mdb' points to a "valid" (struct hfs_mdb).
* Postconditions:
@@ -284,12 +286,19 @@ int hfs_mdb_get(struct super_block *sb)
* If 'backup' is non-zero then the alternate MDB is also written
* and the function doesn't return until it is actually on disk.
*/
-void hfs_mdb_commit(struct super_block *sb)
+int hfs_mdb_commit(struct super_block *sb)
{
struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
+ int ret = 0;
if (sb_rdonly(sb))
- return;
+ return 0;
+
+ if (!buffer_uptodate(HFS_SB(sb)->mdb_bh)) {
+ pr_err("primary MDB is corrupt, mounting read-only\n");
+ sb->s_flags |= SB_RDONLY;
+ return -EIO;
+ }
lock_buffer(HFS_SB(sb)->mdb_bh);
if (test_and_clear_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags)) {
@@ -314,6 +323,13 @@ void hfs_mdb_commit(struct super_block *sb)
* files grow. */
if (test_and_clear_bit(HFS_FLG_ALT_MDB_DIRTY, &HFS_SB(sb)->flags) &&
HFS_SB(sb)->alt_mdb) {
+ if (!buffer_uptodate(HFS_SB(sb)->alt_mdb_bh)) {
+ pr_err("alternate MDB is corrupt, mounting read-only\n");
+ sb->s_flags |= SB_RDONLY;
+ ret = -EIO;
+ goto out;
+ }
+
hfs_inode_write_fork(HFS_SB(sb)->ext_tree->inode, mdb->drXTExtRec,
&mdb->drXTFlSize, NULL);
hfs_inode_write_fork(HFS_SB(sb)->cat_tree->inode, mdb->drCTExtRec,
@@ -360,7 +376,9 @@ void hfs_mdb_commit(struct super_block *sb)
size -= len;
}
}
+out:
unlock_buffer(HFS_SB(sb)->mdb_bh);
+ return ret;
}
void hfs_mdb_close(struct super_block *sb)
@@ -368,6 +386,10 @@ void hfs_mdb_close(struct super_block *sb)
/* update volume attributes */
if (sb_rdonly(sb))
return;
+
+ if (!buffer_uptodate(HFS_SB(sb)->mdb_bh))
+ return;
+
HFS_SB(sb)->mdb->drAtrb |= cpu_to_be16(HFS_SB_ATTRIB_UNMNT);
HFS_SB(sb)->mdb->drAtrb &= cpu_to_be16(~HFS_SB_ATTRIB_INCNSTNT);
mark_buffer_dirty(HFS_SB(sb)->mdb_bh);
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index a466c401f6bb..67eca6f80746 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -35,8 +35,7 @@ MODULE_LICENSE("GPL");
static int hfs_sync_fs(struct super_block *sb, int wait)
{
is_hfs_cnid_counts_valid(sb);
- hfs_mdb_commit(sb);
- return 0;
+ return hfs_mdb_commit(sb);
}
/*
--
2.43.0