* [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
@ 2020-06-15 3:59 Sahitya Tummala
2020-06-15 5:00 ` Eric Biggers
0 siblings, 1 reply; 7+ messages in thread
From: Sahitya Tummala @ 2020-06-15 3:59 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu, Eric Biggers, Satya Tangirala, linux-f2fs-devel
Cc: linux-kernel
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 <stummala@codeaurora.org>
---
This fix is rebased to the tip of fscrypt git -
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
branch - inline-encryption
fs/f2fs/data.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 0dfa8d3..3b53554 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,
- struct page *page)
+static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
+ struct page *page, int *bio_needs_submit)
{
+ struct f2fs_sb_info *sbi = fio->sbi;
enum temp_type temp;
bool found = false;
int ret = -EAGAIN;
@@ -780,6 +781,15 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
continue;
found = true;
+ if (*bio && (!page_is_mergeable(sbi, *bio,
+ *fio->last_block, fio->new_blkaddr) ||
+ !f2fs_crypt_mergeable_bio(*bio,
+ fio->page->mapping->host,
+ fio->page->index, fio))) {
+ ret = 0;
+ *bio_needs_submit = 1;
+ break;
+ }
if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
PAGE_SIZE) {
@@ -864,6 +874,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
struct bio *bio = *fio->bio;
struct page *page = fio->encrypted_page ?
fio->encrypted_page : fio->page;
+ int bio_needs_submit = 0;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
__is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
@@ -872,11 +883,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,8 +892,12 @@ 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, &bio_needs_submit))
+ goto alloc_new;
+ if (bio_needs_submit) {
+ f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
goto alloc_new;
+ }
}
if (fio->io_wbc)
--
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.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 3:59 [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context Sahitya Tummala
@ 2020-06-15 5:00 ` Eric Biggers
2020-06-15 6:16 ` Satya Tangirala via Linux-f2fs-devel
2020-06-15 9:48 ` Sahitya Tummala
0 siblings, 2 replies; 7+ messages in thread
From: Eric Biggers @ 2020-06-15 5:00 UTC (permalink / raw)
To: Sahitya Tummala
Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, Satya Tangirala
On Mon, Jun 15, 2020 at 09:29:48AM +0530, Sahitya Tummala wrote:
> 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 <stummala@codeaurora.org>
> ---
> This fix is rebased to the tip of fscrypt git -
> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> branch - inline-encryption
>
> fs/f2fs/data.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 0dfa8d3..3b53554 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,
> - struct page *page)
> +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
> + struct page *page, int *bio_needs_submit)
> {
> + struct f2fs_sb_info *sbi = fio->sbi;
> enum temp_type temp;
> bool found = false;
> int ret = -EAGAIN;
> @@ -780,6 +781,15 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> continue;
>
> found = true;
> + if (*bio && (!page_is_mergeable(sbi, *bio,
> + *fio->last_block, fio->new_blkaddr) ||
> + !f2fs_crypt_mergeable_bio(*bio,
> + fio->page->mapping->host,
> + fio->page->index, fio))) {
> + ret = 0;
> + *bio_needs_submit = 1;
> + break;
> + }
>
> if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> PAGE_SIZE) {
> @@ -864,6 +874,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> struct bio *bio = *fio->bio;
> struct page *page = fio->encrypted_page ?
> fio->encrypted_page : fio->page;
> + int bio_needs_submit = 0;
>
> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> @@ -872,11 +883,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,8 +892,12 @@ 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, &bio_needs_submit))
> + goto alloc_new;
> + if (bio_needs_submit) {
> + f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
> goto alloc_new;
> + }
> }
>
> if (fio->io_wbc)
Thanks, I'm still trying to understand this part of the code, but it's looking
like this is a real bug. Do you also have a reproducer that produces a KASAN
report, or did you find this another way?
One comment: add_ipu_page() already submits the bio if it's full. Wouldn't it
be better to use that instead of f2fs_submit_merged_ipu_write()? I.e.:
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e9dcda80e599..d7a51dbe208b 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;
}
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 5:00 ` Eric Biggers
@ 2020-06-15 6:16 ` Satya Tangirala via Linux-f2fs-devel
2020-06-15 9:53 ` Sahitya Tummala
2020-06-15 9:48 ` Sahitya Tummala
1 sibling, 1 reply; 7+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-06-15 6:16 UTC (permalink / raw)
To: Eric Biggers; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On Sun, Jun 14, 2020 at 10:00:19PM -0700, Eric Biggers wrote:
> On Mon, Jun 15, 2020 at 09:29:48AM +0530, Sahitya Tummala wrote:
> > 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 <stummala@codeaurora.org>
> > ---
> > This fix is rebased to the tip of fscrypt git -
> > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> > branch - inline-encryption
> >
> > fs/f2fs/data.c | 26 ++++++++++++++++++--------
> > 1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 0dfa8d3..3b53554 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,
> > - struct page *page)
> > +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
> > + struct page *page, int *bio_needs_submit)
> > {
> > + struct f2fs_sb_info *sbi = fio->sbi;
> > enum temp_type temp;
> > bool found = false;
> > int ret = -EAGAIN;
> > @@ -780,6 +781,15 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> > continue;
> >
> > found = true;
> > + if (*bio && (!page_is_mergeable(sbi, *bio,
> > + *fio->last_block, fio->new_blkaddr) ||
> > + !f2fs_crypt_mergeable_bio(*bio,
> > + fio->page->mapping->host,
> > + fio->page->index, fio))) {
> > + ret = 0;
> > + *bio_needs_submit = 1;
> > + break;
> > + }
> >
> > if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> > PAGE_SIZE) {
> > @@ -864,6 +874,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> > struct bio *bio = *fio->bio;
> > struct page *page = fio->encrypted_page ?
> > fio->encrypted_page : fio->page;
> > + int bio_needs_submit = 0;
> >
> > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> > __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> > @@ -872,11 +883,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,8 +892,12 @@ 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, &bio_needs_submit))
> > + goto alloc_new;
> > + if (bio_needs_submit) {
> > + f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
> > goto alloc_new;
> > + }
> > }
> >
> > if (fio->io_wbc)
>
> Thanks, I'm still trying to understand this part of the code, but it's looking
> like this is a real bug. Do you also have a reproducer that produces a KASAN
> report, or did you find this another way?
>
> One comment: add_ipu_page() already submits the bio if it's full. Wouldn't it
> be better to use that instead of f2fs_submit_merged_ipu_write()? I.e.:
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e9dcda80e599..d7a51dbe208b 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;
> }
>
Thanks a lot for looking into this Sahitya! After reading the ipu code,
I do think it's a bug. Regarding the patch itself, I was going to type
out basically the same suggestion as Eric, so I definitely second his
proposal :).
Should I fold this change into the original patch? Or keep it as a
separate patch when I send out the fscrypt/f2fs inline encryption
patches?
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 5:00 ` Eric Biggers
2020-06-15 6:16 ` Satya Tangirala via Linux-f2fs-devel
@ 2020-06-15 9:48 ` Sahitya Tummala
1 sibling, 0 replies; 7+ messages in thread
From: Sahitya Tummala @ 2020-06-15 9:48 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, Jaegeuk Kim, Satya Tangirala
Hi Eric,
On Sun, Jun 14, 2020 at 10:00:19PM -0700, Eric Biggers wrote:
> On Mon, Jun 15, 2020 at 09:29:48AM +0530, Sahitya Tummala wrote:
> > 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 <stummala@codeaurora.org>
> > ---
> > This fix is rebased to the tip of fscrypt git -
> > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> > branch - inline-encryption
> >
> > fs/f2fs/data.c | 26 ++++++++++++++++++--------
> > 1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 0dfa8d3..3b53554 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,
> > - struct page *page)
> > +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
> > + struct page *page, int *bio_needs_submit)
> > {
> > + struct f2fs_sb_info *sbi = fio->sbi;
> > enum temp_type temp;
> > bool found = false;
> > int ret = -EAGAIN;
> > @@ -780,6 +781,15 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> > continue;
> >
> > found = true;
> > + if (*bio && (!page_is_mergeable(sbi, *bio,
> > + *fio->last_block, fio->new_blkaddr) ||
> > + !f2fs_crypt_mergeable_bio(*bio,
> > + fio->page->mapping->host,
> > + fio->page->index, fio))) {
> > + ret = 0;
> > + *bio_needs_submit = 1;
> > + break;
> > + }
> >
> > if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> > PAGE_SIZE) {
> > @@ -864,6 +874,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> > struct bio *bio = *fio->bio;
> > struct page *page = fio->encrypted_page ?
> > fio->encrypted_page : fio->page;
> > + int bio_needs_submit = 0;
> >
> > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> > __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> > @@ -872,11 +883,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,8 +892,12 @@ 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, &bio_needs_submit))
> > + goto alloc_new;
> > + if (bio_needs_submit) {
> > + f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
> > goto alloc_new;
> > + }
> > }
> >
> > if (fio->io_wbc)
>
> Thanks, I'm still trying to understand this part of the code, but it's looking
> like this is a real bug. Do you also have a reproducer that produces a KASAN
> report, or did you find this another way?
>
This was once reported as a stability issue internally on our downstream kernel
having the new FBE patches. I have then reviewed the code and found the issue path.
There is no consistent way to reproduce this problem.
> One comment: add_ipu_page() already submits the bio if it's full. Wouldn't it
> be better to use that instead of f2fs_submit_merged_ipu_write()? I.e.:
>
Your suggestion looks good to me. I will post a v2.
Thanks,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e9dcda80e599..d7a51dbe208b 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;
> }
>
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 6:16 ` Satya Tangirala via Linux-f2fs-devel
@ 2020-06-15 9:53 ` Sahitya Tummala
2020-06-15 15:47 ` Eric Biggers
0 siblings, 1 reply; 7+ messages in thread
From: Sahitya Tummala @ 2020-06-15 9:53 UTC (permalink / raw)
To: Satya Tangirala; +Cc: linux-kernel, linux-f2fs-devel, Eric Biggers, Jaegeuk Kim
Hi Satya,
On Mon, Jun 15, 2020 at 06:16:33AM +0000, Satya Tangirala wrote:
> On Sun, Jun 14, 2020 at 10:00:19PM -0700, Eric Biggers wrote:
> > On Mon, Jun 15, 2020 at 09:29:48AM +0530, Sahitya Tummala wrote:
> > > 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 <stummala@codeaurora.org>
> > > ---
> > > This fix is rebased to the tip of fscrypt git -
> > > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> > > branch - inline-encryption
> > >
> > > fs/f2fs/data.c | 26 ++++++++++++++++++--------
> > > 1 file changed, 18 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 0dfa8d3..3b53554 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,
> > > - struct page *page)
> > > +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
> > > + struct page *page, int *bio_needs_submit)
> > > {
> > > + struct f2fs_sb_info *sbi = fio->sbi;
> > > enum temp_type temp;
> > > bool found = false;
> > > int ret = -EAGAIN;
> > > @@ -780,6 +781,15 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> > > continue;
> > >
> > > found = true;
> > > + if (*bio && (!page_is_mergeable(sbi, *bio,
> > > + *fio->last_block, fio->new_blkaddr) ||
> > > + !f2fs_crypt_mergeable_bio(*bio,
> > > + fio->page->mapping->host,
> > > + fio->page->index, fio))) {
> > > + ret = 0;
> > > + *bio_needs_submit = 1;
> > > + break;
> > > + }
> > >
> > > if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> > > PAGE_SIZE) {
> > > @@ -864,6 +874,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> > > struct bio *bio = *fio->bio;
> > > struct page *page = fio->encrypted_page ?
> > > fio->encrypted_page : fio->page;
> > > + int bio_needs_submit = 0;
> > >
> > > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> > > __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> > > @@ -872,11 +883,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,8 +892,12 @@ 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, &bio_needs_submit))
> > > + goto alloc_new;
> > > + if (bio_needs_submit) {
> > > + f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
> > > goto alloc_new;
> > > + }
> > > }
> > >
> > > if (fio->io_wbc)
> >
> > Thanks, I'm still trying to understand this part of the code, but it's looking
> > like this is a real bug. Do you also have a reproducer that produces a KASAN
> > report, or did you find this another way?
> >
> > One comment: add_ipu_page() already submits the bio if it's full. Wouldn't it
> > be better to use that instead of f2fs_submit_merged_ipu_write()? I.e.:
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index e9dcda80e599..d7a51dbe208b 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;
> > }
> >
> Thanks a lot for looking into this Sahitya! After reading the ipu code,
> I do think it's a bug. Regarding the patch itself, I was going to type
> out basically the same suggestion as Eric, so I definitely second his
> proposal :).
>
> Should I fold this change into the original patch? Or keep it as a
> separate patch when I send out the fscrypt/f2fs inline encryption
> patches?
It may be good to keep it seperate as we already have the base FBE patches in
several downstream kernels, so this fix can be applied easily. But I will
leave it up to you to take a call on this.
Thanks,
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 9:53 ` Sahitya Tummala
@ 2020-06-15 15:47 ` Eric Biggers
2020-06-16 1:36 ` Sahitya Tummala
0 siblings, 1 reply; 7+ messages in thread
From: Eric Biggers @ 2020-06-15 15:47 UTC (permalink / raw)
To: Sahitya Tummala
Cc: Jaegeuk Kim, linux-f2fs-devel, linux-kernel, Satya Tangirala
On Mon, Jun 15, 2020 at 03:23:16PM +0530, Sahitya Tummala wrote:
> >
> > Should I fold this change into the original patch? Or keep it as a
> > separate patch when I send out the fscrypt/f2fs inline encryption
> > patches?
>
> It may be good to keep it seperate as we already have the base FBE patches in
> several downstream kernels, so this fix can be applied easily. But I will
> leave it up to you to take a call on this.
>
We should fold it in because the patch this fixes isn't applied upstream yet.
We'll need to submit this as a separate fix to the Android common kernels
because they already have a previous version of the inline encryption patchset.
(I assume that's where you have the code from.) But that doesn't affect what we
do upstream.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
2020-06-15 15:47 ` Eric Biggers
@ 2020-06-16 1:36 ` Sahitya Tummala
0 siblings, 0 replies; 7+ messages in thread
From: Sahitya Tummala @ 2020-06-16 1:36 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-kernel, linux-f2fs-devel, Jaegeuk Kim, Satya Tangirala
On Mon, Jun 15, 2020 at 08:47:20AM -0700, Eric Biggers wrote:
> On Mon, Jun 15, 2020 at 03:23:16PM +0530, Sahitya Tummala wrote:
> > >
> > > Should I fold this change into the original patch? Or keep it as a
> > > separate patch when I send out the fscrypt/f2fs inline encryption
> > > patches?
> >
> > It may be good to keep it seperate as we already have the base FBE patches in
> > several downstream kernels, so this fix can be applied easily. But I will
> > leave it up to you to take a call on this.
> >
>
> We should fold it in because the patch this fixes isn't applied upstream yet.
>
> We'll need to submit this as a separate fix to the Android common kernels
> because they already have a previous version of the inline encryption patchset.
> (I assume that's where you have the code from.) But that doesn't affect what we
> do upstream.
Sure, no concerns from my side. It can be merged into the original series.
Thanks,
>
> - Eric
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-16 1:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 3:59 [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context Sahitya Tummala
2020-06-15 5:00 ` Eric Biggers
2020-06-15 6:16 ` Satya Tangirala via Linux-f2fs-devel
2020-06-15 9:53 ` Sahitya Tummala
2020-06-15 15:47 ` Eric Biggers
2020-06-16 1:36 ` Sahitya Tummala
2020-06-15 9:48 ` Sahitya Tummala
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).