Re: [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock()

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

On 5/27/24 11:19 AM, lei lu wrote:
> code in ocfs2_search_dirblock:
> if (de_buf + namelen <= dlimit &&
>     ocfs2_match(namelen, name, de)) {
> 
> I'm not sure what "de_buf + namelen < dlimit" means. I think "de_buf +
> namelen" maybe "<= dlimit", but "((struct ocfs2_dir_entry *) de_buf)->name
> + namelen" maybe "> dlimit". Or maybe we should check "de->name + namelen
> <= dlimit" here?
> 

Yes, I think you are right.
de->name + namelen is a more precisely check.

Thanks,
Joseph

> lei lu <llfamsec@gmail.com> 于2024年5月27日周一 11:11写道：
> 
>>> It seems that this check can be moved to:
>>> while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) {
>>>        ...
>>> }
>>
>> Right, it also work.
>>
>>> dlimit = de_buf + bytes;
>>> So here you mean check de_len < bytes?
>>>
>>> Or should we check more precisely, like in ocfs2_check_dir_entry():
>>> (de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len)
>>
>> "de_buf + OCFS2_DIR_MEMBER_LEN > dlimit" checks the fixed members don't
>> stray beyond valid memory region, and "de_buf + de_len > dlimit" for the
>> whole de. You should mean "((char *) de - bh->b_data) + de_len > dlimit"
>> which is similar to "((char *) de - bh->b_data) + rlen >
>> dir->i_sb->s_blocksize)" in ocfs2_check_dir_entry. But dlimit which is end
>> of the de list may not reach the block end.
>>
>>> This is already done in ocfs2_check_dir_entry().
>>
>> Right, We need to check "de_len < OCFS2_DIR_REC_LEN(de->name_len)" before
>> "ocfs2_match(namelen, name, de)", because if de->name_len is too large, I
>> think maybe trigger anther OOB in ocfs2_match. Or we should call
>> ocfs2_check_dir_entry earlier to do a full check for de.
>>
>> Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月27日周一 10:33写道：
>>
>>>
>>>
>>> On 5/26/24 7:06 PM, lei lu wrote:
>>>> Add a check to make sure all members of the ocfs2_dir_entry
>>>> don't stray beyond valid memory region.
>>>>
>>>> Signed-off-by: lei lu <llfamsec@gmail.com>
>>>> ---
>>>>  fs/ocfs2/dir.c | 12 +++++++++++-
>>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
>>>> index d620d4c53c6f..385576d86983 100644
>>>> --- a/fs/ocfs2/dir.c
>>>> +++ b/fs/ocfs2/dir.c
>>>> @@ -358,6 +358,17 @@ static inline int ocfs2_search_dirblock(struct
>>> buffer_head *bh,
>>>>
>>>>               de = (struct ocfs2_dir_entry *) de_buf;
>>>>
>>>> +             if (unlikely(de_buf + OCFS2_DIR_MEMBER_LEN > dlimit)) {
>>>> +                     ret = -1;
>>>> +                     goto bail;
>>>> +             }
>>>
>>> It seems that this check can be moved to:
>>> while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) {
>>>         ...
>>> }
>>>
>>>> +             de_len = le16_to_cpu(de->rec_len);
>>>> +             if (unlikely(de_buf + de_len > dlimit) ||
>>>
>>> dlimit = de_buf + bytes;
>>> So here you mean check de_len < bytes?
>>>
>>> Or should we check more precisely, like in ocfs2_check_dir_entry():
>>> (de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len)
>>>
>>>> +                 unlikely(de_len < OCFS2_DIR_REC_LEN(de->name_len))) {
>>>
>>> This is already done in ocfs2_check_dir_entry().
>>>
>>>> +                     ret = -1;
>>>> +                     goto bail;
>>>> +             }
>>>> +
>>>>               if (de_buf + namelen <= dlimit &&
>>>>                   ocfs2_match(namelen, name, de)) {
>>>>                       /* found a match - just to be sure, do a full
>>> check */
>>>> @@ -371,7 +382,6 @@ static inline int ocfs2_search_dirblock(struct
>>> buffer_head *bh,
>>>>               }
>>>>
>>>>               /* prevent looping on a bad block */
>>>> -             de_len = le16_to_cpu(de->rec_len);
>>>>               if (de_len <= 0) {
>>>>                       ret = -1;
>>>>                       goto bail;
>>>
>>
>