There could be a potential race between these two paths below,
leading to use-after-free when accessing bio->bi_crypt_context.
f2fs_write_cache_pages
->f2fs_do_write_data_page on page#1
->f2fs_inplace_write_data
->f2fs_merge_page_bio
->add_bio_entry
->f2fs_do_write_data_page on page#2
->f2fs_inplace_write_data
->f2fs_merge_page_bio
->f2fs_crypt_mergeable_bio
->fscrypt_mergeable_bio
f2fs_write_begin on page#1
->f2fs_wait_on_page_writeback
->f2fs_submit_merged_ipu_write
->__submit_bio
The bio gets completed, calling
bio_endio
->bio_uninit
->bio_crypt_free_ctx
->use-after-free issue
Fix this by moving f2fs_crypt_mergeable_bio() check within
add_ipu_page() so that it's done under bio_list_lock to prevent
the above race.
Fixes: 15e76ad23e72 ("f2fs: add inline encryption support")
Signed-off-by: Sahitya Tummala <[email protected]>
---
v3:
- remove duplicate bio_add_page(), which was missed in v2 by mistake
v2:
- simplify the logic as per Eric's suggestion to submit the bio in
add_ipu_page() itself instead of using f2fs_submit_merged_ipu_write()
fs/f2fs/data.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 0dfa8d3..ea543f6 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -762,9 +762,10 @@ static void del_bio_entry(struct bio_entry *be)
kmem_cache_free(bio_entry_slab, be);
}
-static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
+static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
struct page *page)
{
+ struct f2fs_sb_info *sbi = fio->sbi;
enum temp_type temp;
bool found = false;
int ret = -EAGAIN;
@@ -780,14 +781,18 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
continue;
found = true;
-
- if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
- PAGE_SIZE) {
+ if (page_is_mergeable(sbi, *bio, *fio->last_block,
+ fio->new_blkaddr) &&
+ f2fs_crypt_mergeable_bio(*bio,
+ fio->page->mapping->host,
+ fio->page->index, fio) &&
+ bio_add_page(*bio, page, PAGE_SIZE, 0) ==
+ PAGE_SIZE) {
ret = 0;
break;
}
- /* bio is full */
+ /* page can't be merged into bio; submit the bio */
del_bio_entry(be);
__submit_bio(sbi, *bio, DATA);
break;
@@ -872,11 +877,6 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
trace_f2fs_submit_page_bio(page, fio);
f2fs_trace_ios(fio, 0);
- if (bio && (!page_is_mergeable(fio->sbi, bio, *fio->last_block,
- fio->new_blkaddr) ||
- !f2fs_crypt_mergeable_bio(bio, fio->page->mapping->host,
- fio->page->index, fio)))
- f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
alloc_new:
if (!bio) {
bio = __bio_alloc(fio, BIO_MAX_PAGES);
@@ -886,7 +886,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
add_bio_entry(fio->sbi, bio, page, fio->temp);
} else {
- if (add_ipu_page(fio->sbi, &bio, page))
+ if (add_ipu_page(fio, &bio, page))
goto alloc_new;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.