[f2fs-dev] [PATCH 0/1] f2fs: check output position in move range ioctl

[f2fs-dev] [PATCH 0/1] f2fs: check output position in move range ioctl

[Dan Robertson]

If a negative value is provided as the output position to the
F2FS_IOC_MOVE_RANGE ioctl, f2fs_get_dnode_of_data may hit a memory
bug like the following:

BUG: unable to handle page fault for address: ffffed10b30435a4
[...]
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009) ...
[...]
Call Trace:
 f2fs_get_dnode_of_data+0xa68/0xde0
[...]
 f2fs_reserve_block+0x3b/0x230
 f2fs_get_new_data_page+0xf0/0x8b0
 ? f2fs_get_lock_data_page+0x1f0/0x1f0
 ? rwsem_down_write_slowpath+0x8d0/0x8d0
 ? rwsem_down_read_slowpath+0x830/0x830
 ? ___might_sleep+0xba/0xd0
 ? f2fs_get_lock_data_page+0x17a/0x1f0
 __exchange_data_block+0x11bf/0x24d0
 ? f2fs_ioc_release_volatile_write+0x170/0x170
 ? __might_sleep+0x31/0xd0
 ? ___might_sleep+0xba/0xd0
 ? rwsem_down_read_slowpath+0x830/0x830
 ? __init_rwsem+0xa0/0xa0
 f2fs_ioctl+0x469c/0x6980

I am reletively inexperienced with F2FS, so I may have missed something,
but I think a simple check of the output position to ensuring that it
isn't a negative value should resolve this issue. The simple test
reproducer I wrote no longer triggers the memmory error after the patch.
Comments and feedback would be appreciated :-)

Cheers,

 - Dan

Dan Robertson (1):
  f2fs: check output position in move range ioctl

 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)




_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl

[Dan Robertson]

When the move range ioctl is used, check the output position and ensure
that it is a non-negative value. Without this check f2fs_get_dnode_of_data
may hit a memmory bug.

Signed-off-by: Dan Robertson <dan@dlrobertson.com>
---
 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8a422400e824..62f9625299ca 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2798,6 +2798,8 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
 	}
 
 	ret = -EINVAL;
+	if (pos_out < 0)
+		goto out_unlock;
 	if (pos_in + len > src->i_size || pos_in + len < pos_in)
 		goto out_unlock;
 	if (len == 0)



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl

[Chao Yu]

On 2020/8/21 11:47, Dan Robertson wrote:
> When the move range ioctl is used, check the output position and ensure
> that it is a non-negative value. Without this check f2fs_get_dnode_of_data
> may hit a memmory bug.

Nice catch!

> 
> Signed-off-by: Dan Robertson <dan@dlrobertson.com>
> ---
>   fs/f2fs/file.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 8a422400e824..62f9625299ca 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2798,6 +2798,8 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
>   	}
>   
>   	ret = -EINVAL;
> +	if (pos_out < 0)
> +		goto out_unlock;

IMO, it would be better to check both pos_in and pos_out out of inode lock
coverage, maybe after encryption flag check?

Thanks,

>   	if (pos_in + len > src->i_size || pos_in + len < pos_in)
>   		goto out_unlock;
>   	if (len == 0)
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl

[Chao Yu]

On 2020/8/21 20:54, Dan Robertson wrote:
> On Fri, Aug 21, 2020 at 05:03:34PM +0800, Chao Yu wrote:
>> On 2020/8/21 11:47, Dan Robertson wrote:
>>> When the move range ioctl is used, check the output position and ensure
>>> that it is a non-negative value. Without this check f2fs_get_dnode_of_data
>>> may hit a memmory bug.
>>
>> Nice catch!
> 
> I noticed that the pos_out value is unsigned in the file_move_range structure,
> but f2fs_file_move_range uses the signed loff_t. Would a better fix be to

singed loff_t looks good to me, as it keeps line with position variable's type
in vfs.

Thanks,

> change the pos_out argument to an unsigned type or ensure that it is cast to
> an unsigned type before we shift it right by F2FS_BLKSIZE_BITS? This also would
> remove the bug, as the index would no longer be negative for the get_node_path
> call in f2fs_get_dnode_of_data, and therefore we should calculate the correct
> offset.
> 
>>
>>>
>>> Signed-off-by: Dan Robertson <dan@dlrobertson.com>
>>> ---
>>>    fs/f2fs/file.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 8a422400e824..62f9625299ca 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -2798,6 +2798,8 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
>>>    	}
>>>    
>>>    	ret = -EINVAL;
>>> +	if (pos_out < 0)
>>> +		goto out_unlock;
>>
>> IMO, it would be better to check both pos_in and pos_out out of inode lock
>> coverage, maybe after encryption flag check?
> 
> Yeah that makes sense. If the pos_in check is the right direction, I'll
> move the check, test it, and set a second patchset version.
> 
> Cheers,
> 
>   - Dan
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel