* [f2fs-dev] [PATCH 0/1] f2fs: check output position in move range ioctl @ 2020-08-21 3:47 Dan Robertson 2020-08-21 3:47 ` [f2fs-dev] [PATCH 1/1] " Dan Robertson 0 siblings, 1 reply; 4+ messages in thread From: Dan Robertson @ 2020-08-21 3:47 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: Dan Robertson, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl 2020-08-21 3:47 [f2fs-dev] [PATCH 0/1] f2fs: check output position in move range ioctl Dan Robertson @ 2020-08-21 3:47 ` Dan Robertson 2020-08-21 9:03 ` Chao Yu 0 siblings, 1 reply; 4+ messages in thread From: Dan Robertson @ 2020-08-21 3:47 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: Dan Robertson, linux-f2fs-devel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl 2020-08-21 3:47 ` [f2fs-dev] [PATCH 1/1] " Dan Robertson @ 2020-08-21 9:03 ` Chao Yu [not found] ` <20200821125404.GA20793@nessie> 0 siblings, 1 reply; 4+ messages in thread From: Chao Yu @ 2020-08-21 9:03 UTC (permalink / raw) To: Dan Robertson, Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20200821125404.GA20793@nessie>]
* Re: [f2fs-dev] [PATCH 1/1] f2fs: check output position in move range ioctl [not found] ` <20200821125404.GA20793@nessie> @ 2020-08-25 3:33 ` Chao Yu 0 siblings, 0 replies; 4+ messages in thread From: Chao Yu @ 2020-08-25 3:33 UTC (permalink / raw) To: Dan Robertson; +Cc: Jaegeuk Kim, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-25 3:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-21 3:47 [f2fs-dev] [PATCH 0/1] f2fs: check output position in move range ioctl Dan Robertson 2020-08-21 3:47 ` [f2fs-dev] [PATCH 1/1] " Dan Robertson 2020-08-21 9:03 ` Chao Yu [not found] ` <20200821125404.GA20793@nessie> 2020-08-25 3:33 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).