Re: [PATCH V3] ocfs2: add a sanity check for i_size

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/16/24 7:57 PM, Edward Adam Davis wrote:
> On Wed, 16 Oct 2024 19:08:26 +0800, Joseph Qi wrote:
>> On 10/16/24 11:52 AM, Edward Adam Davis wrote:
>>> On Wed, 16 Oct 2024 10:06:27 +0800, Joseph Qi wrote:
>>>>> Reported-and-tested-by: syzbot+797d4829dafe3f11dce7@syzkaller.appspotmail.com
>>>>> Closes: https://syzkaller.appspot.com/bug?extid=797d4829dafe3f11dce7
>>>>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>>>>> ---
>>>>> V1 -> V2: keep rc to 0 when falgs contains READHEAD
>>>>> V2 -> V3: check i_size only and alert subject and comments
>>>>>
>>>>>  fs/ocfs2/extent_map.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> #syz test
>>>>>
>>>>> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
>>>>> index f7672472fa82..29d27a70dbdd 100644
>>>>> --- a/fs/ocfs2/extent_map.c
>>>>> +++ b/fs/ocfs2/extent_map.c
>>>>> @@ -961,13 +961,17 @@ int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr,
>>>>>  	int rc = 0;
>>>>>  	u64 p_block, p_count;
>>>>>  	int i, count, done = 0;
>>>>> +	loff_t i_size = i_size_read(inode);
>>>>>
>>>>>  	trace_ocfs2_read_virt_blocks(
>>>>>  	     inode, (unsigned long long)v_block, nr, bhs, flags,
>>>>>  	     validate);
>>>>>
>>>>> +	if (!i_size)
>>>>> +		return -EINVAL;
>>>>> +
>>>>
>>>> Take a more consideration, inode size 0 doesn't mean it has no blocks,
>>>> since we have a case that fallocate with KEEP_SIZE.
>>>> Could you please check inode->i_blocks in above coredump?
>>> I have previously verified the value of inode->i_blocks in my testing environment, which is 0.
>>>
>> So it seems the check condition should be:
>>
>>   (v_block + nr) > (inode->i_blocks >> (inode->i_sb->s_blocksize_bits - 9))
> Do you mean like this?
> 
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index f7672472fa82..9613cd356ac5 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -966,6 +966,9 @@ int ocfs2_read_virt_blocks(struct inode *inode, u64 v_block, int nr,
>              inode, (unsigned long long)v_block, nr, bhs, flags,
>              validate);
> 
> +       if ((v_block + nr) > (inode->i_blocks >> (inode->i_sb->s_blocksize_bits - 9)))
> +               return -EINVAL;
> +
>         if (((v_block + nr - 1) << inode->i_sb->s_blocksize_bits) >=
>             i_size_read(inode)) {
>                 BUG_ON(!(flags & OCFS2_BH_READAHEAD));
> 
Right, which mean the block read request is invalid.