From: Sahitya Tummala <stummala@codeaurora.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
Eric Biggers <ebiggers@kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
Date: Mon, 15 Jun 2020 15:23:16 +0530 [thread overview]
Message-ID: <20200615095316.GB2916@codeaurora.org> (raw)
In-Reply-To: <20200615061633.GA23467@google.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Sahitya Tummala <stummala@codeaurora.org>
To: Satya Tangirala <satyat@google.com>
Cc: Eric Biggers <ebiggers@kernel.org>,
Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>,
linux-f2fs-devel@lists.sourceforge.net,
linux-kernel@vger.kernel.org, stummala@codeaurora.org
Subject: Re: [PATCH] f2fs: fix use-after-free when accessing bio->bi_crypt_context
Date: Mon, 15 Jun 2020 15:23:16 +0530 [thread overview]
Message-ID: <20200615095316.GB2916@codeaurora.org> (raw)
In-Reply-To: <20200615061633.GA23467@google.com>
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.
next prev parent reply other threads:[~2020-06-15 9:53 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-06-15 9:53 ` Sahitya Tummala
2020-06-15 15:47 ` [f2fs-dev] " Eric Biggers
2020-06-15 15:47 ` Eric Biggers
2020-06-16 1:36 ` [f2fs-dev] " Sahitya Tummala
2020-06-16 1:36 ` Sahitya Tummala
2020-06-15 9:48 ` [f2fs-dev] " Sahitya Tummala
2020-06-15 9:48 ` Sahitya Tummala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200615095316.GB2916@codeaurora.org \
--to=stummala@codeaurora.org \
--cc=ebiggers@kernel.org \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=satyat@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.