[lvc-project] [PATCH 3/3] ext2: refuse to return a non-uptodate bh from read_block_bitmap()

[Fedor Pchelkin]

On Tue, 12. May 15:22, Denis Zubov wrote:
> bh_read() can return >= 0 while leaving the buffer not uptodate on
> some failure paths. read_block_bitmap() previously ignored this and
> returned the bh anyway, which let callers operate on unreliable
> b_data.
> 
> Treat !buffer_uptodate() after bh_read() as a hard failure: release
> the buffer and return NULL so callers take the existing io_error
> path. This complements the bg_block_bitmap range check and makes
> read_block_bitmap() reject all known ways of producing a buffer
> whose data cannot be trusted.

Есть основания полагать, что используете ЛЛМ для генерации описания и/или
diff патчей.  Это не упрёк и не что-либо подобное (может быть вы его и не
используете вовсе, знать не могу), но на всякий случай предупрежу - эта
штука может даже закоренелых мэйнтейнеров уболтать в самой что ни на есть
заковыристой форме, не говоря уже о нас с вами.


Не понимаю, в чём решаемая патчем проблема, как вы её встретили и
отладили.  Вроде как bh_read() делает всё от неё зависящее для возврата
uptodate буфера, в ином случае она выдаёт отрицательный статус ошибки,
который в этом коде обрабатывается.

> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 8b91582500ae ("ext2: retry block allocation if new blocks are allocated from system zone")
> Signed-off-by: Denis Zubov <d.zubov at tssltd.ru>
> ---
>  fs/ext2/balloc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index 10e3b100d390..06045aec68d4 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -168,6 +168,20 @@ read_block_bitmap(struct super_block *sb, unsigned int block_group)
>  		return NULL;
>  	}
>  
> +	/*
> +	 * bh_read could return 0 ("already in cache") or 1 ("read OK"),
> +	 * but the buffer may still be !uptodate if the underlying I/O
> +	 * silently failed. In that case b_data is unreliable and using

Как то что выше соотносится с собственно реализацией этой функции?

/* Returns 1 if buffer uptodated, 0 on success, and -EIO on error. */
static inline int bh_read(struct buffer_head *bh, blk_opf_t op_flags)
{
	if (bh_uptodate_or_lock(bh))
		return 1;
	return __bh_read(bh, op_flags, true);
}

/**
 * __bh_read - Submit read for a locked buffer
 * @bh: struct buffer_head
 * @op_flags: appending REQ_OP_* flags besides REQ_OP_READ
 * @wait: wait until reading finish
 *
 * Returns zero on success or don't wait, and -EIO on error.
 */
int __bh_read(struct buffer_head *bh, blk_opf_t op_flags, bool wait)
{
	int ret = 0;

	BUG_ON(!buffer_locked(bh));

	get_bh(bh);
	bh->b_end_io = end_buffer_read_sync;
	submit_bh(REQ_OP_READ | op_flags, bh);
	if (wait) {
		wait_on_buffer(bh);
		if (!buffer_uptodate(bh))
			ret = -EIO;
	}
	return ret;
}
EXPORT_SYMBOL(__bh_read);

> +	 * it leads to UAF/OOB in ext2_try_to_allocate / ext2_free_blocks.
> +	 */
> +	if (!buffer_uptodate(bh)) {
> +		brelse(bh);
> +		ext2_error(sb, __func__,
> +			   "Block bitmap not uptodate - block_group = %d, block_bitmap = %u",
> +			   block_group, le32_to_cpu(desc->bg_block_bitmap));
> +		return NULL;
> +	}
> +
>  	ext2_valid_block_bitmap(sb, desc, block_group, bh);
>  	/*
>  	 * file system mounted not to panic on error, continue with corrupt
> -- 
> 2.53.0