NACK/Cmnt: [PATCH] f2fs: fix a dead loop in f2fs_fiemap() Content-Type:

[Stefan Bader]

On 26.07.2018 13:41, Juerg Haefliger wrote:
> On Wed, 25 Jul 2018 17:39:27 +0200
> Stefan Bader <stefan.bader at canonical.com> wrote:
> 
>> On 19.07.2018 18:35, Colin King wrote:
>>> From: Colin Ian King <colin.king at canonical.com>
>>>
>>> CVE-2017-18257
>>>
>>> A dead loop can be triggered in f2fs_fiemap() using the test case
>>> as below:
>>>
>>>         ...
>>>         fd = open();
>>>         fallocate(fd, 0, 0, 4294967296);
>>>         ioctl(fd, FS_IOC_FIEMAP, fiemap_buf);
>>>         ...
>>>
>>> It's caused by an overflow in __get_data_block():
>>>         ...
>>>         bh->b_size = map.m_len << inode->i_blkbits;
>>>         ...
>>> map.m_len is an unsigned int, and bh->b_size is a size_t which is
>>> 64 bits on 64 bits archtecture, type conversion from an unsigned
>>> int to a size_t will result in an overflow.
>>>
>>> In the above-mentioned case, bh->b_size will be zero, and
>>> f2fs_fiemap() will call get_data_block() at block 0 again an again.
>>>
>>> Fix this by adding a force conversion before left shift.
>>>
>>> Signed-off-by: Wei Fang <fangwei1 at huawei.com>
>>> Acked-by: Chao Yu <yuchao0 at huawei.com>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk at kernel.org>
>>> (backported from upstream commit
>>> b86e33075ed1909d8002745b56ecf73b833db143) Signed-off-by: Colin Ian
>>> King <colin.king at canonical.com> ---  
>>
>> I think that Trusty actually is just not affected and the tracker
>> needs to have hinting adjusted. fiemap looks to be only supported
>> since v3.16:
> 
> OK.
> 
> 
>> Author: Jaegeuk Kim <jaegeuk at kernel.org>
>> Date:   Sun Jun 8 04:30:14 2014 +0900
>>
>>     f2fs: support f2fs_fiemap
>>
>> The replaced line actually has both blkbits and i as ints, so no
>> conversion happens there.
> 
> But if the left-shifted value is bigger than 32bit we get an overflow
> without casting. Or am I missing something?

Hm, no, you are right. I was mis-reading which part was 64bit. Still hard to say
whether it is possible to call get_data_block_ro() in any way that lets i and
blkbits have values that overflow. And then the result has to be used to make
incremental progress ... I cannot say it never happens but is that then covered
by this cve still?

-Stefan

> 
> ...Juerg
> 
>  
>> -Stefan
>>
>>>  fs/f2fs/data.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index aa3438c..884bccc 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode
>>> *inode, sector_t iblock, != (dn.data_blkaddr + i)) || maxblocks ==
>>> i) break;
>>>  		map_bh(bh_result, inode->i_sb, dn.data_blkaddr);
>>> -		bh_result->b_size = (i << blkbits);
>>> +		bh_result->b_size = ((u64)i << blkbits);
>>>  	}
>>>  	f2fs_put_dnode(&dn);
>>>  	trace_f2fs_get_data_block(inode, iblock, bh_result, 0);
>>>   
>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180726/e713542f/attachment.sig>