Re: [PATCH v2 1/2] ocfs2: bound-check dir entries in the readdir re-validation scan

[Joseph Qi]

On 8/11/26 10:43 AM, Zhan Xusheng wrote:
> When the inode version changed since the last readdir(),
> ocfs2_dir_foreach_blk_el() re-scans the directory block from its start to
> relocate the current position:
> 
> 	for (i = 0; i < sb->s_blocksize && i < offset; ) {
> 		de = (struct ocfs2_dir_entry *)(bh->b_data + i);
> 		if (le16_to_cpu(de->rec_len) < OCFS2_DIR_REC_LEN(1))
> 			break;
> 		i += le16_to_cpu(de->rec_len);
> 	}
> 
> i walks the block on rec_len values taken from the block itself and the
> only thing tested is that rec_len is not too small, so a single bogus
> rec_len
> leaves i anywhere in the block, including its last OCFS2_DIR_REC_LEN(1) - 1
> bytes.  @offset comes from ctx->pos, which userspace moves with lseek() on
> the directory fd, and decides how far the walk gets.
> 
> Two bounds are missing, both of which ocfs2_check_dir_entry() applies for
> the emit loop below.
> 
> de->rec_len sits at byte offset 8 within the entry, so dereferencing de in
> that tail reads past the s_blocksize buffer.  ocfs2_check_dir_entry()
> declines to look at an entry that close to the end:
> 
> 	size - buf_offset < OCFS2_DIR_REC_LEN(1)
> 
> Nothing bounds i += rec_len either, so i can end up past the block.  The
> emit loop that follows is guarded by offset < sb->s_blocksize and does not
> run, but
> 
> 	offset = i;
> 	ctx->pos = (ctx->pos & ~((loff_t)sb->s_blocksize - 1)) | offset;
> 
> runs first and ORs a value with bits above the block mask into ctx->pos,
> corrupting the block number readdir() resumes from.
> ocfs2_check_dir_entry() rejects that as "directory entry overrun":
> 
> 	next_offset = buf_offset + rlen;
> 	... next_offset > size
> 
> Apply both bounds.  For a consistent directory this changes nothing:
> entries are at least OCFS2_DIR_REC_LEN(1) bytes and do not cross the end of
> the block, so no valid entry is skipped.
> 
> Found by the sashiko review tool; fix approach suggested by Joseph Qi.
> 
> Link: https://sashiko.dev/#/patchset/20260806022044.167962-1-zhanxusheng@xxxxxxxxxx
> Suggested-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
> Cc: Mark Fasheh <mark@xxxxxxxxxx>
> Cc: Joel Becker <jlbec@xxxxxxxxxxxx>
> Cc: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
> Cc: Changwei Ge <gechangwei@xxxxxxx>
> Cc: Jun Piao <piaojun@xxxxxxxxxx>
> Cc: Heming Zhao <heming.zhao@xxxxxxxx>
> Signed-off-by: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>

Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>

> ---
>  fs/ocfs2/dir.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
> index c30a86856d5b..39ce60874e17 100644
> --- a/fs/ocfs2/dir.c
> +++ b/fs/ocfs2/dir.c
> @@ -1903,7 +1903,10 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode,
>  		 * dirent right now.  Scan from the start of the block
>  		 * to make sure. */
>  		if (!inode_eq_iversion(inode, *f_version)) {
> -			for (i = 0; i < sb->s_blocksize && i < offset; ) {
> +			for (i = 0; i + OCFS2_DIR_REC_LEN(1) <= sb->s_blocksize &&
> +			     i < offset;) {
> +				unsigned int rec_len;
> +
>  				de = (struct ocfs2_dir_entry *) (bh->b_data + i);
>  				/* It's too expensive to do a full
>  				 * dirent test each time round this
> @@ -1911,10 +1914,11 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode,
>  				 * least that it is non-zero.  A
>  				 * failure will be detected in the
>  				 * dirent test below. */
> -				if (le16_to_cpu(de->rec_len) <
> -				    OCFS2_DIR_REC_LEN(1))
> +				rec_len = le16_to_cpu(de->rec_len);
> +				if (rec_len < OCFS2_DIR_REC_LEN(1) ||
> +				    i + rec_len > sb->s_blocksize)
>  					break;
> -				i += le16_to_cpu(de->rec_len);
> +				i += rec_len;
>  			}
>  			offset = i;
>  			ctx->pos = (ctx->pos & ~((loff_t)sb->s_blocksize - 1))