[lvc-project] [PATCH] ext2: fix use-after-free in read_block_bitmap()

[Fedor Pchelkin]

On Sun, 26. Apr 12:17, Denis Zubov wrote:
> When bh_read() returns 0 (buffer already uptodate), read_block_bitmap()
> skips ext2_valid_block_bitmap() and returns the buffer_head without
> validation. On a corrupt filesystem with an invalid block bitmap this
> allows a corrupt bh to propagate to ext2_try_to_allocate(), where
> ext2_set_bit_atomic() writes to bitmap_bh->b_data of an already freed
> page, causing a use-after-free write.
> 
> The comment 'continue with corrupt bitmap' suggests this was intentional,
> however continuing with an invalid bitmap leads to memory corruption.
> The correct behavior on a corrupt filesystem is to return NULL so all
> callers handle the error safely via the existing io_error path.
> 
> Fix by always running ext2_valid_block_bitmap() before returning bh,
> regardless of whether the buffer was cached or freshly read, and
> returning NULL with brelse() on validation failure.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Signed-off-by: Denis Zubov <d.zubov at tssltd.ru>

Для фикс патчей нужен тег Fixes, позволяющий понять коммит, после которого
возможны условия для возникновения ошибки.
https://portal.linuxtesting.ru/How-to-send-patches-to-kernel.html#Fixes

> ---
>  fs/ext2/balloc.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index b8cfab8f98b9..3345a25b4bde 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -146,23 +146,19 @@ read_block_bitmap(struct super_block *sb, unsigned int block_group)
>  		return NULL;
>  	}
>  	ret = bh_read(bh, 0);
> -	if (ret > 0)
> -		return bh;
> -	if (ret < 0) {
> -		brelse(bh);
> -		ext2_error(sb, __func__,
> -			    "Cannot read block bitmap - "
> -			    "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
> -	 * bitmap
> -	 */
> -	return bh;

Просьба всегда прогонять scripts/checkpatch.pl на патч.  В патче серьёзные
проблемы с отступами, нужно настроить текстовый редактор для работы с
кодом ядра.

  perl scripts/checkpatch.pl /path/to/patch/file

ERROR: code indent should use tabs where possible
#54: FILE: fs/ext2/balloc.c:149:
+        if (ret < 0) {$

WARNING: please, no spaces at the start of a line
#54: FILE: fs/ext2/balloc.c:149:
+        if (ret < 0) {$

ERROR: code indent should use tabs where possible
#55: FILE: fs/ext2/balloc.c:150:
+                brelse(bh);$

WARNING: please, no spaces at the start of a line
#55: FILE: fs/ext2/balloc.c:150:
+                brelse(bh);$

ERROR: code indent should use tabs where possible
#56: FILE: fs/ext2/balloc.c:151:
+                ext2_error(sb, __func__,$

...

total: 13 errors, 14 warnings, 1 checks, 36 lines checked


> +        if (ret < 0) {
> +                brelse(bh);
> +                ext2_error(sb, __func__,
> +                            "Cannot read block bitmap - "
> +                            "block_group = %d, block_bitmap = %u",
> +                            block_group, le32_to_cpu(desc->bg_block_bitmap));
> +                return NULL;
> +        }
> +        if (!ext2_valid_block_bitmap(sb, desc, block_group, bh)) {
> +                brelse(bh);
> +                return NULL;
> +        }
> +        return bh;

По сути наполовину откатываете изменения коммита 8b91582500ae ("ext2:
retry block allocation if new blocks are allocated from system zone") — он
их всё же для какой-то обозначенной цели делал, поэтому прежде чем
что-либо менять, следует в этом разобраться.

При встрече ошибок/повреждений файловой системы ext2 (и многих других)
есть обычно три варианта, определяемые пользователем:

* errors=continue - вариант Сизколлера, когда при обнаружении повреждения
  метаданных драйвер файловой системы громко ругается в лог ядра (может и
  без WARNING), но продолжает своё функционирование дальше;
* errors=remount-ro - при обнаружении повреждений фс перемонтируется как
  только для чтения, что позволяет снизить вероятность дальнейших проблем;
* errors=panic - ядро запаникует.

В случае проверок внутри ext2_valid_block_bitmap() эти три варианта
выбираются, см. ext2_error().


Пока что на основании коммита 8b91582500ae можно полагать, что файловая
система по крайней мере в этом месте не должна возвращать ошибку о
повреждении наверх.

Следует понять, что именно в поврежденной битовой карте ведёт уже к
проблеме KASAN - возможно какую-то дополнительную обработку нужно делать
ближе к тому месту.

>  }
>  
>  static void group_adjust_blocks(struct super_block *sb, int group_no,
> -- 
> 2.53.0