[PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Sheng Yong]

sb_getblk does not guarantee the buffer head is uptodate. If bh is not
uptodate, the data (may be used as boot code) in area before
F2FS_SUPER_OFFSET may get corrupted when super block is committed.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/f2fs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8173ae688814..7def48cb2b22 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2334,7 +2334,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
 	}
 
 	/* write back-up superblock first */
-	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
+	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
 	if (!bh)
 		return -EIO;
 	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
@@ -2345,7 +2345,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
 		return err;
 
 	/* write current valid superblock */
-	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
+	bh = sb_bread(sbi->sb, sbi->valid_super_block);
 	if (!bh)
 		return -EIO;
 	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
-- 
2.11.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Chao Yu]

On 2018/1/29 16:04, Sheng Yong wrote:
> sb_getblk does not guarantee the buffer head is uptodate. If bh is not
> uptodate, the data (may be used as boot code) in area before

Why boot code can be stored into the position f2fs superblock locates?

And even we have updated the buffer, we will still commit superblock's
datas into that buffer, boot code can be messed up anyway, right?

Thanks,

> F2FS_SUPER_OFFSET may get corrupted when super block is committed.
> 
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>  fs/f2fs/super.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 8173ae688814..7def48cb2b22 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2334,7 +2334,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>  	}
>  
>  	/* write back-up superblock first */
> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
> +	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
>  	if (!bh)
>  		return -EIO;
>  	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
> @@ -2345,7 +2345,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>  		return err;
>  
>  	/* write current valid superblock */
> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
> +	bh = sb_bread(sbi->sb, sbi->valid_super_block);
>  	if (!bh)
>  		return -EIO;
>  	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Sheng Yong]

Hi, Chao

On 2018/1/29 16:27, Chao Yu wrote:
> On 2018/1/29 16:04, Sheng Yong wrote:
>> sb_getblk does not guarantee the buffer head is uptodate. If bh is not
>> uptodate, the data (may be used as boot code) in area before
> 
> Why boot code can be stored into the position f2fs superblock locates?
According to Ext4_Disk_Layout [1], the first 1024 bytes is used to install
boot sectors (may be used by some legacy bootloader, I'm not quite sure :(
> 
> And even we have updated the buffer, we will still commit superblock's
> datas into that buffer, boot code can be messed up anyway, right?
The first 1024 bytes are all set to 0 by mkfs, while __f2fs_commit_super only
updates bh->b_data from F2FS_SUPER_OFFSET, but not the whole 4KB area. If the
buffer head is not read from super block (4KB), there is a chance it may get
messed up.

Thanks,
Sheng
> 

[1] https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout
> Thanks,
> 
>> F2FS_SUPER_OFFSET may get corrupted when super block is committed.
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>   fs/f2fs/super.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 8173ae688814..7def48cb2b22 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -2334,7 +2334,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>   	}
>>   
>>   	/* write back-up superblock first */
>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
>>   	if (!bh)
>>   		return -EIO;
>>   	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>> @@ -2345,7 +2345,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>   		return err;
>>   
>>   	/* write current valid superblock */
>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block);
>>   	if (!bh)
>>   		return -EIO;
>>   	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>>
> 
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Chao Yu]

Hi Sheng Yong,

On 2018/1/29 16:39, Sheng Yong wrote:
> Hi, Chao
> 
> On 2018/1/29 16:27, Chao Yu wrote:
>> On 2018/1/29 16:04, Sheng Yong wrote:
>>> sb_getblk does not guarantee the buffer head is uptodate. If bh is not
>>> uptodate, the data (may be used as boot code) in area before
>>
>> Why boot code can be stored into the position f2fs superblock locates?
> According to Ext4_Disk_Layout [1], the first 1024 bytes is used to install
> boot sectors (may be used by some legacy bootloader, I'm not quite sure :(

OK, I can understand your concern now, anyway, no matter the area is used
by someone else or not, let's keep data in that area as it is as possible
as we can.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

>>
>> And even we have updated the buffer, we will still commit superblock's
>> datas into that buffer, boot code can be messed up anyway, right?
> The first 1024 bytes are all set to 0 by mkfs, while __f2fs_commit_super only
> updates bh->b_data from F2FS_SUPER_OFFSET, but not the whole 4KB area. If the
> buffer head is not read from super block (4KB), there is a chance it may get
> messed up.
> 
> Thanks,
> Sheng
>>
> 
> [1] https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout
>> Thanks,
>>
>>> F2FS_SUPER_OFFSET may get corrupted when super block is committed.
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>> ---
>>>   fs/f2fs/super.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 8173ae688814..7def48cb2b22 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -2334,7 +2334,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>>   	}
>>>   
>>>   	/* write back-up superblock first */
>>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
>>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
>>>   	if (!bh)
>>>   		return -EIO;
>>>   	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>>> @@ -2345,7 +2345,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>>   		return err;
>>>   
>>>   	/* write current valid superblock */
>>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
>>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block);
>>>   	if (!bh)
>>>   		return -EIO;
>>>   	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>>>
>>
>>
>> .
>>
> 
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Sheng Yong]

On 2018/1/29 16:58, Chao Yu wrote:
> Hi Sheng Yong,
> 
> On 2018/1/29 16:39, Sheng Yong wrote:
>> Hi, Chao
>>
>> On 2018/1/29 16:27, Chao Yu wrote:
>>> On 2018/1/29 16:04, Sheng Yong wrote:
>>>> sb_getblk does not guarantee the buffer head is uptodate. If bh is not
>>>> uptodate, the data (may be used as boot code) in area before
>>>
>>> Why boot code can be stored into the position f2fs superblock locates?
>> According to Ext4_Disk_Layout [1], the first 1024 bytes is used to install
>> boot sectors (may be used by some legacy bootloader, I'm not quite sure :(
> 
> OK, I can understand your concern now, anyway, no matter the area is used
> by someone else or not, let's keep data in that area as it is as possible
> as we can.

I checked the change again, I think we should also remove the extra
set_buffer_uptodate(bh); in __f2fs_commit_super like the following,

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8173ae688814..07d72d651c45 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1894,7 +1894,6 @@ static int __f2fs_commit_super(struct buffer_head *bh,
  	lock_buffer(bh);
  	if (super)
  		memcpy(bh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
-	set_buffer_uptodate(bh);
  	set_buffer_dirty(bh);
  	unlock_buffer(bh);

I will resend it, thanks.

> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
> 
>>>
>>> And even we have updated the buffer, we will still commit superblock's
>>> datas into that buffer, boot code can be messed up anyway, right?
>> The first 1024 bytes are all set to 0 by mkfs, while __f2fs_commit_super only
>> updates bh->b_data from F2FS_SUPER_OFFSET, but not the whole 4KB area. If the
>> buffer head is not read from super block (4KB), there is a chance it may get
>> messed up.
>>
>> Thanks,
>> Sheng
>>>
>>
>> [1] https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout
>>> Thanks,
>>>
>>>> F2FS_SUPER_OFFSET may get corrupted when super block is committed.
>>>>
>>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>>> ---
>>>>    fs/f2fs/super.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 8173ae688814..7def48cb2b22 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -2334,7 +2334,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>>>    	}
>>>>    
>>>>    	/* write back-up superblock first */
>>>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
>>>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
>>>>    	if (!bh)
>>>>    		return -EIO;
>>>>    	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>>>> @@ -2345,7 +2345,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>>>    		return err;
>>>>    
>>>>    	/* write current valid superblock */
>>>> -	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
>>>> +	bh = sb_bread(sbi->sb, sbi->valid_super_block);
>>>>    	if (!bh)
>>>>    		return -EIO;
>>>>    	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
> 
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH v2] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Sheng Yong]

sb_getblk does not guarantee the buffer head is uptodate. If bh is not
uptodate, the data (may be used as boot code) in area before
F2FS_SUPER_OFFSET may get corrupted when super block is committed.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/f2fs/super.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8173ae688814..3db1f02ed8c2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1894,7 +1894,6 @@ static int __f2fs_commit_super(struct buffer_head *bh,
 	lock_buffer(bh);
 	if (super)
 		memcpy(bh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
-	set_buffer_uptodate(bh);
 	set_buffer_dirty(bh);
 	unlock_buffer(bh);
 
@@ -2334,7 +2333,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
 	}
 
 	/* write back-up superblock first */
-	bh = sb_getblk(sbi->sb, sbi->valid_super_block ? 0: 1);
+	bh = sb_bread(sbi->sb, sbi->valid_super_block ? 0 : 1);
 	if (!bh)
 		return -EIO;
 	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
@@ -2345,7 +2344,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
 		return err;
 
 	/* write current valid superblock */
-	bh = sb_getblk(sbi->sb, sbi->valid_super_block);
+	bh = sb_bread(sbi->sb, sbi->valid_super_block);
 	if (!bh)
 		return -EIO;
 	err = __f2fs_commit_super(bh, F2FS_RAW_SUPER(sbi));
-- 
2.11.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v2] f2fs: fix potential corruption in area before F2FS_SUPER_OFFSET

[Chao Yu]

On 2018/1/29 19:13, Sheng Yong wrote:
> sb_getblk does not guarantee the buffer head is uptodate. If bh is not
> uptodate, the data (may be used as boot code) in area before
> F2FS_SUPER_OFFSET may get corrupted when super block is committed.
> 
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot