From 34b47e3d73a21ef992905746cdb044ce02d3b29a Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Sat, 5 Apr 2025 12:26:43 -0400 Subject: bcachefs: Fix UAF in bchfs_read() Commit 3ba0240a8789 fixed a bug in the read retry path in __bch2_read(), and changed bchfs_read() to match - to avoid a landmine if bch2_read_extent() ever starts returning transaction restarts. But that was incorrect, because bchfs_read() doesn't use a separate stack allocated bvec_iter, it uses the one in the rbio being submitted. Add a comment explaining the issue, and revert the buggy change. Fixes: 3ba0240a8789 ("bcachefs: Fix silent short reads in data read retry path") Reported-by: syzbot+2deb10b8dc9aae6fab67@syzkaller.appspotmail.com Signed-off-by: Kent Overstreet --- fs/bcachefs/fs-io-buffered.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/bcachefs/fs-io-buffered.c b/fs/bcachefs/fs-io-buffered.c index 19d4599918dc..e3a75dcca60c 100644 --- a/fs/bcachefs/fs-io-buffered.c +++ b/fs/bcachefs/fs-io-buffered.c @@ -225,11 +225,26 @@ static void bchfs_read(struct btree_trans *trans, bch2_read_extent(trans, rbio, iter.pos, data_btree, k, offset_into_extent, flags); - swap(rbio->bio.bi_iter.bi_size, bytes); + /* + * Careful there's a landmine here if bch2_read_extent() ever + * starts returning transaction restarts here. + * + * We've changed rbio->bi_iter.bi_size to be "bytes we can read + * from this extent" with the swap call, and we restore it + * below. That restore needs to come before checking for + * errors. + * + * But unlike __bch2_read(), we use the rbio bvec iter, not one + * on the stack, so we can't do the restore right after the + * bch2_read_extent() call: we don't own that iterator anymore + * if BCH_READ_last_fragment is set, since we may have submitted + * that rbio instead of cloning it. + */ if (flags & BCH_READ_last_fragment) break; + swap(rbio->bio.bi_iter.bi_size, bytes); bio_advance(&rbio->bio, bytes); err: if (ret && -- cgit