[f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Sahitya Tummala]

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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Satya Tangirala via 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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Sahitya Tummala]

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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Sahitya Tummala]

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

Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context

[Sahitya Tummala]

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