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

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

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;