Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
* [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
@ 2019-08-02 10:15 Chao Yu
  2019-08-06  0:42 ` Jaegeuk Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2019-08-02 10:15 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel

As Pavel Machek reported:

"We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
good idea to report it to the syslog and mark filesystem as "needing
fsck" if filesystem can do that."

Still we need improve the original patch with:
- use unlikely keyword
- add message print
- return EUCLEAN

However, after rethink this patch, I don't think we should add such
condition check here as below reasons:
- We have already checked the field in f2fs_sanity_check_ckpt(),
- If there is fs corrupt or security vulnerability, there is nothing
to guarantee the field is integrated after the check, unless we do
the check before each of its use, however no filesystem does that.
- We only have similar check for bitmap, which was added due to there
is bitmap corruption happened on f2fs' runtime in product.
- There are so many key fields in SB/CP/NAT did have such check
after f2fs_sanity_check_{sb,cp,..}.

So I propose to revert this unneeded check.

This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9693fa4c8971..2eff9c008ae0 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
 		seg_i = CURSEG_I(sbi, i);
 		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
 		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
-		if (blk_off > ENTRIES_IN_SUM) {
-			f2fs_bug_on(sbi, 1);
-			f2fs_put_page(page, 1);
-			return -EFAULT;
-		}
 		seg_i->next_segno = segno;
 		reset_curseg(sbi, i, 0);
 		seg_i->alloc_type = ckpt->alloc_type[i];
-- 
2.18.0.rc1



_______________________________________________
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] 6+ messages in thread

* Re: [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
  2019-08-02 10:15 [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access" Chao Yu
@ 2019-08-06  0:42 ` Jaegeuk Kim
  2019-08-06  1:10   ` Chao Yu
  0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2019-08-06  0:42 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 08/02, Chao Yu wrote:
> As Pavel Machek reported:
> 
> "We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
> good idea to report it to the syslog and mark filesystem as "needing
> fsck" if filesystem can do that."
> 
> Still we need improve the original patch with:
> - use unlikely keyword
> - add message print
> - return EUCLEAN
> 
> However, after rethink this patch, I don't think we should add such
> condition check here as below reasons:
> - We have already checked the field in f2fs_sanity_check_ckpt(),
> - If there is fs corrupt or security vulnerability, there is nothing
> to guarantee the field is integrated after the check, unless we do
> the check before each of its use, however no filesystem does that.
> - We only have similar check for bitmap, which was added due to there
> is bitmap corruption happened on f2fs' runtime in product.
> - There are so many key fields in SB/CP/NAT did have such check
> after f2fs_sanity_check_{sb,cp,..}.
> 
> So I propose to revert this unneeded check.

IIRC, this came from security vulnerability report which can access
out-of-boundary memory region. Could you write another patch to address the
above issues?

> 
> This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/segment.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 9693fa4c8971..2eff9c008ae0 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
>  		seg_i = CURSEG_I(sbi, i);
>  		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
>  		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
> -		if (blk_off > ENTRIES_IN_SUM) {
> -			f2fs_bug_on(sbi, 1);
> -			f2fs_put_page(page, 1);
> -			return -EFAULT;
> -		}
>  		seg_i->next_segno = segno;
>  		reset_curseg(sbi, i, 0);
>  		seg_i->alloc_type = ckpt->alloc_type[i];
> -- 
> 2.18.0.rc1


_______________________________________________
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] 6+ messages in thread

* Re: [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
  2019-08-06  0:42 ` Jaegeuk Kim
@ 2019-08-06  1:10   ` Chao Yu
  2019-08-06  1:28     ` Jaegeuk Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2019-08-06  1:10 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2019/8/6 8:42, Jaegeuk Kim wrote:
> On 08/02, Chao Yu wrote:
>> As Pavel Machek reported:
>>
>> "We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
>> good idea to report it to the syslog and mark filesystem as "needing
>> fsck" if filesystem can do that."
>>
>> Still we need improve the original patch with:
>> - use unlikely keyword
>> - add message print
>> - return EUCLEAN
>>
>> However, after rethink this patch, I don't think we should add such
>> condition check here as below reasons:
>> - We have already checked the field in f2fs_sanity_check_ckpt(),
>> - If there is fs corrupt or security vulnerability, there is nothing
>> to guarantee the field is integrated after the check, unless we do
>> the check before each of its use, however no filesystem does that.
>> - We only have similar check for bitmap, which was added due to there
>> is bitmap corruption happened on f2fs' runtime in product.
>> - There are so many key fields in SB/CP/NAT did have such check
>> after f2fs_sanity_check_{sb,cp,..}.
>>
>> So I propose to revert this unneeded check.
> 
> IIRC, this came from security vulnerability report which can access

I don't think that's correct report, since we have checked validation of that
field during mount, if it can be ruined after that, any variables can't be trusted.

Now we just check bitmaps at real-time, because we have encountered such bitmap
corruption in product.

Thanks,

> out-of-boundary memory region. Could you write another patch to address the
> above issues?
> 
>>
>> This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/segment.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 9693fa4c8971..2eff9c008ae0 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
>>  		seg_i = CURSEG_I(sbi, i);
>>  		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
>>  		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
>> -		if (blk_off > ENTRIES_IN_SUM) {
>> -			f2fs_bug_on(sbi, 1);
>> -			f2fs_put_page(page, 1);
>> -			return -EFAULT;
>> -		}
>>  		seg_i->next_segno = segno;
>>  		reset_curseg(sbi, i, 0);
>>  		seg_i->alloc_type = ckpt->alloc_type[i];
>> -- 
>> 2.18.0.rc1
> .
> 


_______________________________________________
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] 6+ messages in thread

* Re: [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
  2019-08-06  1:10   ` Chao Yu
@ 2019-08-06  1:28     ` Jaegeuk Kim
  2019-08-06  2:07       ` Chao Yu
  0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2019-08-06  1:28 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 08/06, Chao Yu wrote:
> On 2019/8/6 8:42, Jaegeuk Kim wrote:
> > On 08/02, Chao Yu wrote:
> >> As Pavel Machek reported:
> >>
> >> "We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
> >> good idea to report it to the syslog and mark filesystem as "needing
> >> fsck" if filesystem can do that."
> >>
> >> Still we need improve the original patch with:
> >> - use unlikely keyword
> >> - add message print
> >> - return EUCLEAN
> >>
> >> However, after rethink this patch, I don't think we should add such
> >> condition check here as below reasons:
> >> - We have already checked the field in f2fs_sanity_check_ckpt(),
> >> - If there is fs corrupt or security vulnerability, there is nothing
> >> to guarantee the field is integrated after the check, unless we do
> >> the check before each of its use, however no filesystem does that.
> >> - We only have similar check for bitmap, which was added due to there
> >> is bitmap corruption happened on f2fs' runtime in product.
> >> - There are so many key fields in SB/CP/NAT did have such check
> >> after f2fs_sanity_check_{sb,cp,..}.
> >>
> >> So I propose to revert this unneeded check.
> > 
> > IIRC, this came from security vulnerability report which can access
> 
> I don't think that's correct report, since we have checked validation of that
> field during mount, if it can be ruined after that, any variables can't be trusted.

I assumed this was reproduced with a fuzzed image.
I'll check it with Ocean.

> 
> Now we just check bitmaps at real-time, because we have encountered such bitmap
> corruption in product.
> 
> Thanks,
> 
> > out-of-boundary memory region. Could you write another patch to address the
> > above issues?
> > 
> >>
> >> This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.
> >>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >>  fs/f2fs/segment.c | 5 -----
> >>  1 file changed, 5 deletions(-)
> >>
> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >> index 9693fa4c8971..2eff9c008ae0 100644
> >> --- a/fs/f2fs/segment.c
> >> +++ b/fs/f2fs/segment.c
> >> @@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
> >>  		seg_i = CURSEG_I(sbi, i);
> >>  		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
> >>  		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
> >> -		if (blk_off > ENTRIES_IN_SUM) {
> >> -			f2fs_bug_on(sbi, 1);
> >> -			f2fs_put_page(page, 1);
> >> -			return -EFAULT;
> >> -		}
> >>  		seg_i->next_segno = segno;
> >>  		reset_curseg(sbi, i, 0);
> >>  		seg_i->alloc_type = ckpt->alloc_type[i];
> >> -- 
> >> 2.18.0.rc1
> > .
> > 


_______________________________________________
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] 6+ messages in thread

* Re: [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
  2019-08-06  1:28     ` Jaegeuk Kim
@ 2019-08-06  2:07       ` Chao Yu
  2019-08-06  3:05         ` Jaegeuk Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2019-08-06  2:07 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2019/8/6 9:28, Jaegeuk Kim wrote:
> On 08/06, Chao Yu wrote:
>> On 2019/8/6 8:42, Jaegeuk Kim wrote:
>>> On 08/02, Chao Yu wrote:
>>>> As Pavel Machek reported:
>>>>
>>>> "We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
>>>> good idea to report it to the syslog and mark filesystem as "needing
>>>> fsck" if filesystem can do that."
>>>>
>>>> Still we need improve the original patch with:
>>>> - use unlikely keyword
>>>> - add message print
>>>> - return EUCLEAN
>>>>
>>>> However, after rethink this patch, I don't think we should add such
>>>> condition check here as below reasons:
>>>> - We have already checked the field in f2fs_sanity_check_ckpt(),
>>>> - If there is fs corrupt or security vulnerability, there is nothing
>>>> to guarantee the field is integrated after the check, unless we do
>>>> the check before each of its use, however no filesystem does that.
>>>> - We only have similar check for bitmap, which was added due to there
>>>> is bitmap corruption happened on f2fs' runtime in product.
>>>> - There are so many key fields in SB/CP/NAT did have such check
>>>> after f2fs_sanity_check_{sb,cp,..}.
>>>>
>>>> So I propose to revert this unneeded check.
>>>
>>> IIRC, this came from security vulnerability report which can access
>>
>> I don't think that's correct report, since we have checked validation of that
>> field during mount, if it can be ruined after that, any variables can't be trusted.
> 
> I assumed this was reproduced with a fuzzed image.

I expect f2fs_sanity_check_ckpt() should reject mounting such fuzzed image.

> I'll check it with Ocean.
> 
>>
>> Now we just check bitmaps at real-time, because we have encountered such bitmap
>> corruption in product.
>>
>> Thanks,
>>
>>> out-of-boundary memory region. Could you write another patch to address the
>>> above issues?
>>>
>>>>
>>>> This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>  fs/f2fs/segment.c | 5 -----
>>>>  1 file changed, 5 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 9693fa4c8971..2eff9c008ae0 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
>>>>  		seg_i = CURSEG_I(sbi, i);
>>>>  		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
>>>>  		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
>>>> -		if (blk_off > ENTRIES_IN_SUM) {
>>>> -			f2fs_bug_on(sbi, 1);
>>>> -			f2fs_put_page(page, 1);
>>>> -			return -EFAULT;
>>>> -		}
>>>>  		seg_i->next_segno = segno;
>>>>  		reset_curseg(sbi, i, 0);
>>>>  		seg_i->alloc_type = ckpt->alloc_type[i];
>>>> -- 
>>>> 2.18.0.rc1
>>> .
>>>
> .
> 


_______________________________________________
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] 6+ messages in thread

* Re: [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access"
  2019-08-06  2:07       ` Chao Yu
@ 2019-08-06  3:05         ` Jaegeuk Kim
  0 siblings, 0 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2019-08-06  3:05 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 08/06, Chao Yu wrote:
> On 2019/8/6 9:28, Jaegeuk Kim wrote:
> > On 08/06, Chao Yu wrote:
> >> On 2019/8/6 8:42, Jaegeuk Kim wrote:
> >>> On 08/02, Chao Yu wrote:
> >>>> As Pavel Machek reported:
> >>>>
> >>>> "We normally use -EUCLEAN to signal filesystem corruption. Plus, it is
> >>>> good idea to report it to the syslog and mark filesystem as "needing
> >>>> fsck" if filesystem can do that."
> >>>>
> >>>> Still we need improve the original patch with:
> >>>> - use unlikely keyword
> >>>> - add message print
> >>>> - return EUCLEAN
> >>>>
> >>>> However, after rethink this patch, I don't think we should add such
> >>>> condition check here as below reasons:
> >>>> - We have already checked the field in f2fs_sanity_check_ckpt(),
> >>>> - If there is fs corrupt or security vulnerability, there is nothing
> >>>> to guarantee the field is integrated after the check, unless we do
> >>>> the check before each of its use, however no filesystem does that.
> >>>> - We only have similar check for bitmap, which was added due to there
> >>>> is bitmap corruption happened on f2fs' runtime in product.
> >>>> - There are so many key fields in SB/CP/NAT did have such check
> >>>> after f2fs_sanity_check_{sb,cp,..}.
> >>>>
> >>>> So I propose to revert this unneeded check.
> >>>
> >>> IIRC, this came from security vulnerability report which can access
> >>
> >> I don't think that's correct report, since we have checked validation of that
> >> field during mount, if it can be ruined after that, any variables can't be trusted.
> > 
> > I assumed this was reproduced with a fuzzed image.
> 
> I expect f2fs_sanity_check_ckpt() should reject mounting such fuzzed image.

It seems I should have reviewed this more carefully. Checking the security
concern one more time.

> 
> > I'll check it with Ocean.
> > 
> >>
> >> Now we just check bitmaps at real-time, because we have encountered such bitmap
> >> corruption in product.
> >>
> >> Thanks,
> >>
> >>> out-of-boundary memory region. Could you write another patch to address the
> >>> above issues?
> >>>
> >>>>
> >>>> This reverts commit 56f3ce675103e3fb9e631cfb4131fc768bc23e9a.
> >>>>
> >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>>> ---
> >>>>  fs/f2fs/segment.c | 5 -----
> >>>>  1 file changed, 5 deletions(-)
> >>>>
> >>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>> index 9693fa4c8971..2eff9c008ae0 100644
> >>>> --- a/fs/f2fs/segment.c
> >>>> +++ b/fs/f2fs/segment.c
> >>>> @@ -3492,11 +3492,6 @@ static int read_compacted_summaries(struct f2fs_sb_info *sbi)
> >>>>  		seg_i = CURSEG_I(sbi, i);
> >>>>  		segno = le32_to_cpu(ckpt->cur_data_segno[i]);
> >>>>  		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[i]);
> >>>> -		if (blk_off > ENTRIES_IN_SUM) {
> >>>> -			f2fs_bug_on(sbi, 1);
> >>>> -			f2fs_put_page(page, 1);
> >>>> -			return -EFAULT;
> >>>> -		}
> >>>>  		seg_i->next_segno = segno;
> >>>>  		reset_curseg(sbi, i, 0);
> >>>>  		seg_i->alloc_type = ckpt->alloc_type[i];
> >>>> -- 
> >>>> 2.18.0.rc1
> >>> .
> >>>
> > .
> > 


_______________________________________________
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] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02 10:15 [f2fs-dev] [PATCH] Revert "f2fs: avoid out-of-range memory access" Chao Yu
2019-08-06  0:42 ` Jaegeuk Kim
2019-08-06  1:10   ` Chao Yu
2019-08-06  1:28     ` Jaegeuk Kim
2019-08-06  2:07       ` Chao Yu
2019-08-06  3:05         ` Jaegeuk Kim

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net linux-f2fs-devel@archiver.kernel.org
	public-inbox-index linux-f2fs-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox