2022-06-23 04:51:16

by CGEL

[permalink] [raw]
Subject: Bug report: ntfs_read_block may crash system

From Zeal Robot <[email protected]>

Hi! Zeal Robot found a potential risky bug about NTFS under the help of syzkaller.
This will cause OS crash when CONFIG_NTFS_FS is set and panic_on_oops is on.

We have a reproducer below and Debug log.

=============================================================================
Debugging log:
=============================================================================
[ 101.384038] kernel BUG at fs/ntfs/aops.c:186!
[ 101.386267] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[ 101.387099] PKRU: 55555554
[ 101.387099] Call Trace:
[ 101.387099] <TASK>
[ 101.387099] ? ntfs_end_buffer_async_read+0xb20/0xb20
[ 101.387099] ? filemap_add_folio+0x115/0x180
[ 101.387099] ? add_to_page_cache_locked+0x90/0x90
[ 101.387099] do_read_cache_folio+0x4d4/0x950
[ 101.387099] ? generic_file_read_iter+0x4f0/0x4f0
[ 101.387099] ? _extract_crng.constprop.0+0xc1/0xf0
[ 101.387099] ? ntfs_attr_find+0x649/0xa90
[ 101.387099] ? __kasan_check_write+0x15/0x20
[ 101.387099] read_cache_page+0x6c/0x150
[ 101.387099] map_mft_record+0x1a5/0x960
[ 101.387099] ntfs_read_locked_inode+0x1a7/0x58b0
[ 101.387099] ? ntfs_mapping_pairs_decompress+0x79e/0x1030
[ 101.387099] ? ntfs_attr_reinit_search_ctx+0x14e/0x430
[ 101.387099] ntfs_read_inode_mount+0xb72/0x2170
[ 101.387099] ntfs_fill_super+0x10f2/0x4420
[ 101.387099] ? snprintf+0xa9/0xd0
[ 101.387099] mount_bdev+0x305/0x3d0
[ 101.387099] ? ntfs_remount+0xc0/0xc0
[ 101.387099] ntfs_mount+0x3b/0x50
[ 101.387099] entry_SYSCALL_64_after_hwframe+0x44/0xae
===============================================================================

The C reproducer of this bug is as follows:
===============================================================================

#define _GNU_SOURCE

#include <endian.h>
#include <errno.h>
#include <fcntl.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>

#include <linux/loop.h>

#ifndef __NR_memfd_create
#define __NR_memfd_create 319
#endif

static unsigned long long procid;

struct fs_image_segment {
void* data;
uintptr_t size;
uintptr_t offset;
};

#define IMAGE_MAX_SEGMENTS 4096
#define IMAGE_MAX_SIZE (129 << 20)

static unsigned long fs_image_segment_check(unsigned long size,
unsigned long nsegs,
struct fs_image_segment* segs)
{
if (nsegs > IMAGE_MAX_SEGMENTS)
nsegs = IMAGE_MAX_SEGMENTS;
for (size_t i = 0; i < nsegs; i++) {
if (segs[i].size > IMAGE_MAX_SIZE)
segs[i].size = IMAGE_MAX_SIZE;
segs[i].offset %= IMAGE_MAX_SIZE;
if (segs[i].offset > IMAGE_MAX_SIZE - segs[i].size)
segs[i].offset = IMAGE_MAX_SIZE - segs[i].size;
if (size < segs[i].offset + segs[i].offset)
size = segs[i].offset + segs[i].offset;
}
if (size > IMAGE_MAX_SIZE)
size = IMAGE_MAX_SIZE;
return size;
}
static int setup_loop_device(long unsigned size, long unsigned nsegs,
struct fs_image_segment* segs,
const char* loopname, int* memfd_p, int* loopfd_p)
{
int err = 0, loopfd = -1;
size = fs_image_segment_check(size, nsegs, segs);
int memfd = syscall(__NR_memfd_create, "syzkaller", 0);
if (memfd == -1) {
err = errno;
goto error;
}
if (ftruncate(memfd, size)) {
err = errno;
goto error_close_memfd;
}
for (size_t i = 0; i < nsegs; i++) {
if (pwrite(memfd, segs[i].data, segs[i].size, segs[i].offset) < 0) {
}
}
loopfd = open(loopname, O_RDWR);
if (loopfd == -1) {
err = errno;
goto error_close_memfd;
}
if (ioctl(loopfd, LOOP_SET_FD, memfd)) {
if (errno != EBUSY) {
err = errno;
goto error_close_loop;
}
ioctl(loopfd, LOOP_CLR_FD, 0);
usleep(1000);
if (ioctl(loopfd, LOOP_SET_FD, memfd)) {
err = errno;
goto error_close_loop;
}
}
*memfd_p = memfd;
*loopfd_p = loopfd;
return 0;

error_close_loop:
close(loopfd);
error_close_memfd:
close(memfd);
error:
errno = err;
return -1;
}

static long syz_mount_image(volatile long fsarg, volatile long dir,
volatile unsigned long size,
volatile unsigned long nsegs,
volatile long segments, volatile long flags,
volatile long optsarg)
{
struct fs_image_segment* segs = (struct fs_image_segment*)segments;
int res = -1, err = 0, loopfd = -1, memfd = -1, need_loop_device = !!segs;
char* mount_opts = (char*)optsarg;
char* target = (char*)dir;
char* fs = (char*)fsarg;
char* source = NULL;
char loopname[64];
if (need_loop_device) {
memset(loopname, 0, sizeof(loopname));
snprintf(loopname, sizeof(loopname), "/dev/loop%llu", procid);
if (setup_loop_device(size, nsegs, segs, loopname, &memfd, &loopfd) == -1) {
printf("setup_loop_device failed\n");
return -1;
}
source = loopname;
}
mkdir(target, 0777);
char opts[256];
memset(opts, 0, sizeof(opts));
if (strlen(mount_opts) > (sizeof(opts) - 32)) {
}
strncpy(opts, mount_opts, sizeof(opts) - 32);
if (strcmp(fs, "iso9660") == 0) {
flags |= MS_RDONLY;
} else if (strncmp(fs, "ext", 3) == 0) {
if (strstr(opts, "errors=panic") || strstr(opts, "errors=remount-ro") == 0)
strcat(opts, ",errors=continue");
} else if (strcmp(fs, "xfs") == 0) {
strcat(opts, ",nouuid");
}

printf("Start to mount...\n");
res = mount(source, target, fs, flags, opts);
if (res == -1) {
printf("mount failed: %s\n", strerror(errno));
err = errno;
goto error_clear_loop;
}
res = open(target, O_RDONLY | O_DIRECTORY);
if (res == -1) {
err = errno;
}

error_clear_loop:
if (need_loop_device) {
ioctl(loopfd, LOOP_CLR_FD, 0);
close(loopfd);
close(memfd);
}
errno = err;
return res;
}

int main(void)
{
syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);

memcpy((void*)0x20000000, "ntfs\000", 5);
memcpy((void*)0x20000100, "./file0\000", 8);
*(uint64_t*)0x20000200 = 0x20010000;
memcpy(
(void*)0x20010000,
"\xeb\x52\x90\x4e\x54\x46\x53\x20\x20\x20\x20\x00\x02\x08\x00\x00\x00\x00"
"\x00\x00\x00\xf8\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x80\x00\x80\x00\xff\x0f\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00"
"\x00\x00\xff\x00\x00\x00\x00\x00\x00\x00\xf6\x00\x00\x00\x01\x00\x00\x00"
"\x3d\xaa\xf5\x5a\xf9\x83\x09\x09\x00\x00\x00\x00\x0e\x1f\xbe\x71\x7c\xac"
"\x22\xc0\x74\x0b\x56\xb4\x0e\xbb\x07\x00\xcd\x10\x5e\xeb\xf0\x32\xe4\xcd"
"\x16\xcd\x19\xeb\xfe\x54\x68\x69\x73\x20\x69\x73\x20\x6e\x6f\x74\x20\x61"
"\x20\x62\x6f\x6f\x74\x61\x62\x6c\x65\x20\x64\x69\x73\x6b\x2e\x20\x50\x6c"
"\x65\x61\x73\x65\x20\x69\x6e\x73\x65\x72\x74\x20\x61\x20\x62\x6f\x6f\x74"
"\x61\x62\x6c\x65\x20\x66\x6c\x6f\x70\x70\x79\x20\x61\x6e\x64\x0d\x0a\x70"
"\x72\x65\x73\x73\x20\x61\x6e\x79\x20\x6b\x65\x79\x20\x74\x6f\x20\x74\x72"
"\x79\x20\x61\x67\x61\x69\x6e\x20\x2e\x2e\x2e\x20\x0d\x0a\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00",
224);
*(uint64_t*)0x20000208 = 0xe0;
*(uint64_t*)0x20000210 = 0;
*(uint64_t*)0x20000218 = 0x20010100;
memcpy((void*)0x20010100,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x55\xaa",
32);
*(uint64_t*)0x20000220 = 0x20;
*(uint64_t*)0x20000228 = 0x1e0;
*(uint64_t*)0x20000230 = 0x20010200;
memcpy((void*)0x20010200,
"\xff\xff\x00\x07\x00\x00\x00\x00\x3f\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
32);
*(uint64_t*)0x20000238 = 0x20;
*(uint64_t*)0x20000240 = 0x2000;
*(uint64_t*)0x20000248 = 0x20010300;
memcpy(
(void*)0x20010300,
"\x46\x49\x4c\x45\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00"
"\x01\x00\x38\x00\x01\x00\x98\x01\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00"
"\x00\x00\x10\x00\x00\x00\x60\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00"
"\x48\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x30\x00\x00\x00\x68\x00\x00\x00\x00\x00"
"\x18\x00\x00\x00\x02\x00\x4a\x00\x00\x00\x18\x00\x01\x00\x05\x00\x00\x00"
"\x00\x00\x05\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x00\x70\x00\x00\x00\x00\x00\x00\x00\x6c\x00\x00\x00\x00\x00\x00\x06\x00"
"\x00\x00\x00\x00\x00\x00\x04\x03\x24\x00\x4d\x00\x46\x00\x54\x00\x00\x00"
"\x00\x00\x00\x00\x80\x00\x00\x00\x48\x00\x00\x00\x01\x00\x40\x00\x00\x00"
"\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x12\x00\x00\x00\x00\x00\x00\x00"
"\x40\x00\x00\x00\x00\x00\x00\x00\x00\x30\x01\x00\x00\x00\x00\x00\x00\x18"
"\x01\x00\x00\x00\x00\x00",
312);
*(uint64_t*)0x20000250 = 0x138;
*(uint64_t*)0x20000258 = 0x4000;
*(uint64_t*)0x20000260 = 0x20010500;
memcpy((void*)0x20010500,
"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
"\000\000\000\000\000\000\000\000\000\000\000\000\000\b",
31);
*(uint64_t*)0x20000268 = 0x1f;
*(uint64_t*)0x20000270 = 0x41e0;
*(uint64_t*)0x20000278 = 0x20010600;
memcpy((void*)0x20010600,
"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
"\000\000\000\000\000\000\000\000\000\000\000\000\000\b",
31);
*(uint64_t*)0x20000280 = 0x1f;
*(uint64_t*)0x20000288 = 0x43e0;
*(uint64_t*)0x20000290 = 0x20010c00;
memcpy(
(void*)0x20010c00,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x46\x49\x4c\x45"
"\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x01\x00\x38\x00"
"\x01\x00\xe0\x01\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x06\x00\x00\x00\x03\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x10\x00"
"\x00\x00\x48\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x30\x00\x00\x00"
"\x18\x00\x00\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x06\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x30\x00"
"\x00\x00\x68\x00\x00\x00\x00\x00\x18\x00\x00\x00\x01\x00\x50\x00\x00\x00"
"\x18\x00\x01\x00\x05\x00\x00\x00\x00\x00\x05\x00\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00\x00\x07\x03\x24\x00"
"\x56\x00\x6f\x00\x6c\x00\x75\x00\x6d\x00\x65\x00\x50\x00\x00\x00\x80\x00"
"\x00\x00\x00\x00\x18\x00\x00\x00\x02\x00\x64\x00\x00\x00\x18\x00\x00\x00"
"\x01\x00\x04\x80\x48\x00\x00\x00\x54\x00\x00\x00\x00\x00\x00\x00\x14\x00"
"\x00\x00\x02\x00\x34\x00\x02\x00\x00\x00\x00\x00\x14\x00\x9f\x01\x12\x00"
"\x01\x01\x00\x00\x00\x00\x00\x05\x12\x00\x00\x00\x00\x00\x18\x00\x9f\x01"
"\x12\x00\x01\x02\x00\x00\x00\x00\x00\x05\x20\x00\x00\x00\x20\x02\x00\x00"
"\x01\x01\x00\x00\x00\x00\x00\x05\x12\x00\x00\x00\x01\x02\x00\x00\x00\x00"
"\x00\x05\x20\x00\x00\x00\x20\x02\x00\x00\x00\x00\x00\x00\x60\x00\x00\x00"
"\x30\x00\x00\x00\x00\x00\x18\x00\x00\x00\x04\x00\x12\x00\x00\x00\x18\x00"
"\x00\x00\x73\x00\x79\x00\x7a\x00\x6b\x00\x61\x00\x6c\x00\x6c\x00\x65\x00"
"\x72\x00\x00\x00\x00\x00\x00\x00\x70\x00\x00\x00\x28\x00\x00\x00\x00\x00"
"\x18\x00\x00\x00\x05\x00\x0c\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x03\x01\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x18\x00"
"\x00\x00\x00\x00\x18\x00\x00\x00\x03\x00\x00\x00\x00\x00\x18\x00\x00\x00"
"\xff\xff\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x02",
543);
*(uint64_t*)0x20000298 = 0x21f;
*(uint64_t*)0x200002a0 = 0x4be0;
*(uint64_t*)0x200002a8 = 0x20010f00;
memcpy((void*)0x20010f00,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02",
31);
*(uint64_t*)0x200002b0 = 0x1f;
*(uint64_t*)0x200002b8 = 0x4fe0;
*(uint64_t*)0x200002c0 = 0x20011200;
memcpy(
(void*)0x20011200,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x46\x49\x4c\x45"
"\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x05\x00\x01\x00\x38\x00"
"\x03\x00\x00\x02\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x06\x00\x00\x00\x05\x00\x00\x00\x07\x00\x00\x00\x00\x00\x00\x00\x10\x00"
"\x00\x00\x48\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x30\x00\x00\x00"
"\x18\x00\x00\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\xbb\xc8\x07\x00\xc8\x8d"
"\xd6\x01\xbb\xc8\x07\x00\xc8\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x26\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x30\x00"
"\x00\x00\x60\x00\x00\x00\x00\x00\x18\x00\x00\x00\x01\x00\x44\x00\x00\x00"
"\x18\x00\x01\x00\x05\x00\x00\x00\x00\x00\x05\x00\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x06\x00\x00\x10\x00\x00\x00\x00\x01\x03\x2e\x00"
"\x00\x00\x00\x00\x50\x00\x00\x00\x48\x00\x00\x00\x01\x00\x40\x00\x00\x00"
"\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00"
"\x40\x00\x00\x00\x00\x00\x00\x00\x00\x20\x00\x00\x00\x00\x00\x00\x2c\x10"
"\x00\x00\x00\x00\x00\x00\x2c\x10\x00\x00\x00\x00\x00\x00\x11\x02\x43\x00"
"\x00\x00\x00\x00\x90\x00\x00\x00\x58\x00\x00\x00\x00\x04\x18\x00\x00\x00"
"\x03\x00\x38\x00\x00\x00\x20\x00\x00\x00\x24\x00\x49\x00\x33\x00\x30\x00"
"\x30\x00\x00\x00\x01\x00\x00\x00\x00\x10\x00\x00\x01\x00\x00\x00\x10\x00"
"\x00\x00\x28\x00\x00\x00\x28\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x18\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\xa0\x00\x00\x00\x50\x00\x00\x00\x01\x04\x40\x00\x00\x00\x05\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x48\x00"
"\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00"
"\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x24\x00\x49\x00\x33\x00"
"\x30\x00\x11\x01\x45\x00\x00\x00\x00\x00\xb0\x00\x00\x00\x28\x00\x00\x00"
"\x00\x04\x18\x00\x00\x00\x04\x00\x08\x00\x00\x00\x20\x00\x00\x00\x24\x00"
"\x49\x00\x33\x00\x30\x00\x01\x00\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff"
"\x00\x00\x07",
543);
*(uint64_t*)0x200002c8 = 0x21f;
*(uint64_t*)0x200002d0 = 0x53e0;
*(uint64_t*)0x200002d8 = 0x20011500;
memcpy(
(void*)0x20011500,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x07\x00\x46\x49\x4c\x45"
"\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x06\x00\x01\x00\x38\x00"
"\x01\x00\x50\x01\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x03\x00\x00\x00\x06\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x10\x00"
"\x00\x00\x60\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x48\x00\x00\x00"
"\x18\x00\x00\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x06\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x30\x00\x00\x00\x68\x00\x00\x00\x00\x00\x18\x00\x00\x00"
"\x02\x00\x50\x00\x00\x00\x18\x00\x01\x00\x05\x00\x00\x00\x00\x00\x05\x00"
"\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f"
"\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x10\x00\x00"
"\x00\x00\x00\x00\x40\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00\x00\x00"
"\x00\x00\x07\x03\x24\x00\x42\x00\x69\x00\x74\x00\x6d\x00\x61\x00\x70\x00"
"\x80\x00\x00\x00\x48\x00\x00\x00\x01\x00\x40\x00\x00\x00\x01\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00"
"\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x40",
337);
*(uint64_t*)0x200002e0 = 0x151;
*(uint64_t*)0x200002e8 = 0x57e0;
*(uint64_t*)0x200002f0 = 0x20011700;
memcpy((void*)0x20011700,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02",
31);
*(uint64_t*)0x200002f8 = 0x1f;
*(uint64_t*)0x20000300 = 0x59e0;
*(uint64_t*)0x20000308 = 0x20011800;
memcpy((void*)0x20011800,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02",
31);
*(uint64_t*)0x20000310 = 0x1f;
*(uint64_t*)0x20000318 = 0x5be0;
*(uint64_t*)0x20000320 = 0x20011e00;
memcpy(
(void*)0x20011e00,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x46\x49\x4c\x45"
"\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x09\x00\x01\x00\x38\x00"
"\x09\x00\xa8\x02\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x05\x00\x00\x00\x09\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x10\x00"
"\x00\x00\x60\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x48\x00\x00\x00"
"\x18\x00\x00\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x06\x00\x00\x20\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x30\x00\x00\x00\x68\x00\x00\x00\x00\x00\x18\x00\x00\x00"
"\x01\x00\x50\x00\x00\x00\x18\x00\x01\x00\x05\x00\x00\x00\x00\x00\x05\x00"
"\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f"
"\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x20\x00\x00"
"\x00\x00\x07\x03\x24\x00\x53\x00\x65\x00\x63\x00\x75\x00\x72\x00\x65\x00"
"\x80\x00\x00\x00\x50\x00\x00\x00\x01\x04\x40\x00\x00\x00\x02\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00\x00\x00\x00\x00\x48\x00\x00\x00"
"\x00\x00\x00\x00\x00\x10\x04\x00\x00\x00\x00\x00\xfc\x00\x04\x00\x00\x00"
"\x00\x00\xfc\x00\x04\x00\x00\x00\x00\x00\x24\x00\x53\x00\x44\x00\x53\x00"
"\x11\x41\x48\x00\x00\x00\x00\x00\x90\x00\x00\x00\xb0\x00\x00\x00\x00\x04"
"\x18\x00\x00\x00\x03\x00\x90\x00\x00\x00\x20\x00\x00\x00\x24\x00\x53\x00"
"\x44\x00\x48\x00\x00\x00\x00\x00\x12\x00\x00\x00\x00\x10\x00\x00\x01\x00"
"\x00\x00\x10\x00\x00\x00\x80\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00"
"\x18\x00\x14\x00\x00\x00\x00\x00\x30\x00\x08\x00\x00\x00\x00\x00\x51\x24"
"\xb3\x00\x01\x01\x00\x00\x51\x24\xb3\x00\x01\x01\x00\x00\x80\x00\x00\x00"
"\x00\x00\x00\x00\x7c\x00\x00\x00\x49\x00\x49\x00\x18\x00\x14\x00\x00\x00"
"\x00\x00\x30\x00\x08\x00\x00\x00\x00\x00\xf0\x12\x03\xf8\x00\x01\x00\x00"
"\xf0\x12\x03\xf8\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x7c\x00"
"\x00\x00\x49\x00\x49\x00\x00\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00"
"\x02\x00\x02",
543);
*(uint64_t*)0x20000328 = 0x21f;
*(uint64_t*)0x20000330 = 0x63e0;
*(uint64_t*)0x20000338 = 0x20012100;
memcpy((void*)0x20012100,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02",
31);
*(uint64_t*)0x20000340 = 0x1f;
*(uint64_t*)0x20000348 = 0x67e0;
*(uint64_t*)0x20000350 = 0x20012400;
memcpy(
(void*)0x20012400,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x46\x49\x4c\x45"
"\x30\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0b\x00\x01\x00\x38\x00"
"\x03\x00\x80\x02\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x03\x00\x00\x00\x0b\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x10\x00"
"\x00\x00\x60\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x48\x00\x00\x00"
"\x18\x00\x00\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x06\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x30\x00\x00\x00\x68\x00\x00\x00\x00\x00\x18\x00\x00\x00"
"\x01\x00\x50\x00\x00\x00\x18\x00\x01\x00\x05\x00\x00\x00\x00\x00\x05\x00"
"\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f"
"\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x10\x00\x00"
"\x00\x00\x07\x03\x24\x00\x45\x00\x78\x00\x74\x00\x65\x00\x6e\x00\x64\x00"
"\x90\x00\x00\x00\x78\x01\x00\x00\x00\x04\x18\x00\x00\x00\x02\x00\x58\x01"
"\x00\x00\x20\x00\x00\x00\x24\x00\x49\x00\x33\x00\x30\x00\x30\x00\x00\x00"
"\x01\x00\x00\x00\x00\x10\x00\x00\x01\x00\x00\x00\x10\x00\x00\x00\x48\x01"
"\x00\x00\x48\x01\x00\x00\x00\x00\x00\x00\x19\x00\x00\x00\x00\x00\x01\x00"
"\x60\x00\x4e\x00\x00\x00\x00\x00\x0b\x00\x00\x00\x00\x00\x0b\x00\x00\x6f"
"\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff"
"\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x26\x00\x00\x20\x00\x00\x00\x00"
"\x06\x03\x24\x00\x4f\x00\x62\x00\x6a\x00\x49\x00\x64\x00\x00\x00\x18\x00"
"\x00\x00\x00\x00\x01\x00\x60\x00\x4e\x00\x00\x00\x00\x00\x0b\x00\x00\x00"
"\x00\x00\x0b\x00\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d"
"\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01\x00\x6f\x95\xff\xc7\x8d\xd6\x01"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x26\x00"
"\x00\x20\x00\x00\x00\x00\x06\x03\x24\x00\x51\x00\x75\x00\x6f\x00\x74\x00"
"\x61\x00\x02",
543);
*(uint64_t*)0x20000358 = 0x21f;
*(uint64_t*)0x20000360 = 0x6be0;
*(uint64_t*)0x20000368 = 0x20012700;
memcpy((void*)0x20012700,
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02",
31);
*(uint64_t*)0x20000370 = 0x1f;
*(uint64_t*)0x20000378 = 0x6fe0;
*(uint8_t*)0x2007dc00 = 0;
syz_mount_image(0x20000000, 0x20000100, 0, 0x10, 0x20000200, 0, 0x2007dc00);
return 0;
}


2022-06-23 08:27:22

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: Bug report: ntfs_read_block may crash system

On Thu, Jun 23, 2022 at 03:36:35AM +0000, xu xin wrote:
> >From Zeal Robot <[email protected]>
>
> Hi! Zeal Robot found a potential risky bug about NTFS under the help of syzkaller.
> This will cause OS crash when CONFIG_NTFS_FS is set and panic_on_oops is on.

Do you have a proposed fix for this as you can easily reproduce this?

And it looks like you need root permisions for this, right?

thanks,

greg k-h

2022-06-23 10:21:34

by CGEL

[permalink] [raw]
Subject: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

From: xu xin <[email protected]>

As the bug description at
https://lore.kernel.org/lkml/[email protected]/
attckers can use this bug to crash the system.

So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
warning to the syslog and return instead until someone really solve
the problem.

Cc: [email protected]
Reported-by: Zeal Robot <[email protected]>
Reported-by: [email protected]
Reviewed-by: Songyi Zhang <[email protected]>
Reviewed-by: Yang Yang <[email protected]>
Reviewed-by: Jiang Xuexin<[email protected]>
Reviewed-by: Zhang wenya<[email protected]>
Signed-off-by: xu xin <[email protected]>
---

Change for v2:
- Use ntfs_warning instead of WARN().
- Add the tag Cc: [email protected].
---
fs/ntfs/aops.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
index 5f4fb6ca6f2e..84d68efb4ace 100644
--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
vol = ni->vol;

/* $MFT/$DATA must have its complete runlist in memory at all times. */
- BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
+ if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
+ ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
+ "and NInoAttr(ni) is null.");
+ unlock_page(page);
+ return -EINVAL;
+ }

blocksize = vol->sb->s_blocksize;
blocksize_bits = vol->sb->s_blocksize_bits;
--
2.25.1

2022-06-23 19:17:11

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

On Thu, Jun 23, 2022 at 09:49:56AM +0000, [email protected] wrote:
> From: xu xin <[email protected]>
>
> As the bug description at
> https://lore.kernel.org/lkml/[email protected]/
> attckers can use this bug to crash the system.
>
> So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
> warning to the syslog and return instead until someone really solve
> the problem.
>
> Cc: [email protected]
> Reported-by: Zeal Robot <[email protected]>
> Reported-by: [email protected]
> Reviewed-by: Songyi Zhang <[email protected]>
> Reviewed-by: Yang Yang <[email protected]>
> Reviewed-by: Jiang Xuexin<[email protected]>
> Reviewed-by: Zhang wenya<[email protected]>
> Signed-off-by: xu xin <[email protected]>
> ---
>
> Change for v2:
> - Use ntfs_warning instead of WARN().
> - Add the tag Cc: [email protected].
> ---
> fs/ntfs/aops.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index 5f4fb6ca6f2e..84d68efb4ace 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
> vol = ni->vol;
>
> /* $MFT/$DATA must have its complete runlist in memory at all times. */
> - BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
> + if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
> + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
> + "and NInoAttr(ni) is null.");
> + unlock_page(page);
> + return -EINVAL;
> + }

A better warning message that doesn't rely on implementation details (struct
field and macro names) would be "Runlist of $MFT/$DATA is not cached". Also,
why does this situation happen in the first place? Is there a way to prevent
this situation in the first place?

- Eric

2022-06-24 02:46:37

by Namjae Jeon

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

2022-06-24 2:08 GMT+09:00, Eric Biggers <[email protected]>:
> On Thu, Jun 23, 2022 at 09:49:56AM +0000, [email protected] wrote:
>> From: xu xin <[email protected]>
>>
>> As the bug description at
>> https://lore.kernel.org/lkml/[email protected]/
>> attckers can use this bug to crash the system.
>>
>> So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
>> warning to the syslog and return instead until someone really solve
>> the problem.
>>
>> Cc: [email protected]
>> Reported-by: Zeal Robot <[email protected]>
>> Reported-by: [email protected]
>> Reviewed-by: Songyi Zhang <[email protected]>
>> Reviewed-by: Yang Yang <[email protected]>
>> Reviewed-by: Jiang Xuexin<[email protected]>
>> Reviewed-by: Zhang wenya<[email protected]>
>> Signed-off-by: xu xin <[email protected]>
>> ---
>>
>> Change for v2:
>> - Use ntfs_warning instead of WARN().
>> - Add the tag Cc: [email protected].
>> ---
>> fs/ntfs/aops.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
>> index 5f4fb6ca6f2e..84d68efb4ace 100644
>> --- a/fs/ntfs/aops.c
>> +++ b/fs/ntfs/aops.c
>> @@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
>> vol = ni->vol;
>>
>> /* $MFT/$DATA must have its complete runlist in memory at all times. */
>> - BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
>> + if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
>> + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
>> + "and NInoAttr(ni) is null.");
>> + unlock_page(page);
>> + return -EINVAL;
>> + }
>
> A better warning message that doesn't rely on implementation details
> (struct
> field and macro names) would be "Runlist of $MFT/$DATA is not cached".
> Also,
> why does this situation happen in the first place? Is there a way to
> prevent
> this situation in the first place?

ntfs_mapping_pairs_decompress() should return error pointer instead of NULL.
Callers is checking error value using IS_ERR(). and the mapping pairs
array of @MFT entry is empty, I think it's corrupted, it should cause
mount failure.

I haven't checked if this patch fix the problem. Xu, Can you check it ?

diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
index 97932fb5179c..31263fe0772f 100644
--- a/fs/ntfs/runlist.c
+++ b/fs/ntfs/runlist.c
@@ -766,8 +766,11 @@ runlist_element
*ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
return ERR_PTR(-EIO);
}
/* If the mapping pairs array is valid but empty, nothing to do. */
- if (!vcn && !*buf)
+ if (!vcn && !*buf) {
+ if (!old_rl)
+ return ERR_PTR(-EIO);
return old_rl;
+ }
/* Current position in runlist array. */
rlpos = 0;
/* Allocate first page and set current runlist size to one page. */

>
> - Eric
>

2022-06-24 04:02:08

by CGEL

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

On Fri, Jun 24, 2022 at 11:33:28AM +0900, Namjae Jeon wrote:
> 2022-06-24 2:08 GMT+09:00, Eric Biggers <[email protected]>:
> > On Thu, Jun 23, 2022 at 09:49:56AM +0000, [email protected] wrote:
> >> From: xu xin <[email protected]>
> >>
> >> As the bug description at
> >> https://lore.kernel.org/lkml/[email protected]/
> >> attckers can use this bug to crash the system.
> >>
> >> So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
> >> warning to the syslog and return instead until someone really solve
> >> the problem.
> >>
> >> Cc: [email protected]
> >> Reported-by: Zeal Robot <[email protected]>
> >> Reported-by: [email protected]
> >> Reviewed-by: Songyi Zhang <[email protected]>
> >> Reviewed-by: Yang Yang <[email protected]>
> >> Reviewed-by: Jiang Xuexin<[email protected]>
> >> Reviewed-by: Zhang wenya<[email protected]>
> >> Signed-off-by: xu xin <[email protected]>
> >> ---
> >>
> >> Change for v2:
> >> - Use ntfs_warning instead of WARN().
> >> - Add the tag Cc: [email protected].
> >> ---
> >> fs/ntfs/aops.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> >> index 5f4fb6ca6f2e..84d68efb4ace 100644
> >> --- a/fs/ntfs/aops.c
> >> +++ b/fs/ntfs/aops.c
> >> @@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
> >> vol = ni->vol;
> >>
> >> /* $MFT/$DATA must have its complete runlist in memory at all times. */
> >> - BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
> >> + if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
> >> + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
> >> + "and NInoAttr(ni) is null.");
> >> + unlock_page(page);
> >> + return -EINVAL;
> >> + }
> >
> > A better warning message that doesn't rely on implementation details
> > (struct
> > field and macro names) would be "Runlist of $MFT/$DATA is not cached".
> > Also,
> > why does this situation happen in the first place? Is there a way to
> > prevent
> > this situation in the first place?
>
> ntfs_mapping_pairs_decompress() should return error pointer instead of NULL.
> Callers is checking error value using IS_ERR(). and the mapping pairs
> array of @MFT entry is empty, I think it's corrupted, it should cause
> mount failure.
>
> I haven't checked if this patch fix the problem. Xu, Can you check it ?
>

I have test it and it fixes the problem.

Thanks.

> diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
> index 97932fb5179c..31263fe0772f 100644
> --- a/fs/ntfs/runlist.c
> +++ b/fs/ntfs/runlist.c
> @@ -766,8 +766,11 @@ runlist_element
> *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
> return ERR_PTR(-EIO);
> }
> /* If the mapping pairs array is valid but empty, nothing to do. */
> - if (!vcn && !*buf)
> + if (!vcn && !*buf) {
> + if (!old_rl)
> + return ERR_PTR(-EIO);
> return old_rl;
> + }
> /* Current position in runlist array. */
> rlpos = 0;
> /* Allocate first page and set current runlist size to one page. */
>
> >
> > - Eric
> >

2022-06-24 14:41:39

by Namjae Jeon

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

2022-06-24 21:19 GMT+09:00, Anton Altaparmakov <[email protected]>:
> Hi,
>
> On 24 Jun 2022, at 03:33, Namjae Jeon
> <[email protected]<mailto:[email protected]>> wrote:
>
> 2022-06-24 2:08 GMT+09:00, Eric Biggers
> <[email protected]<mailto:[email protected]>>:
> On Thu, Jun 23, 2022 at 09:49:56AM +0000,
> [email protected]<mailto:[email protected]> wrote:
> From: xu xin <[email protected]<mailto:[email protected]>>
>
> As the bug description at
> https://lore.kernel.org/lkml/[email protected]/
> attckers can use this bug to crash the system.
>
> So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
> warning to the syslog and return instead until someone really solve
> the problem.
>
> Cc: [email protected]
> Reported-by: Zeal Robot <[email protected]>
> Reported-by: [email protected]
> Reviewed-by: Songyi Zhang <[email protected]>
> Reviewed-by: Yang Yang <[email protected]>
> Reviewed-by: Jiang Xuexin<[email protected]>
> Reviewed-by: Zhang wenya<[email protected]>
> Signed-off-by: xu xin <[email protected]>
> ---
>
> Change for v2:
> - Use ntfs_warning instead of WARN().
> - Add the tag Cc: [email protected].
> ---
> fs/ntfs/aops.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index 5f4fb6ca6f2e..84d68efb4ace 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
> vol = ni->vol;
>
> /* $MFT/$DATA must have its complete runlist in memory at all times. */
> - BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
> + if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
> + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
> + "and NInoAttr(ni) is null.");
> + unlock_page(page);
> + return -EINVAL;
> + }
>
> A better warning message that doesn't rely on implementation details
> (struct
> field and macro names) would be "Runlist of $MFT/$DATA is not cached".
> Also,
> why does this situation happen in the first place? Is there a way to
> prevent
> this situation in the first place?
>
> ntfs_mapping_pairs_decompress() should return error pointer instead of
> NULL.
>
> Callers is checking error value using IS_ERR(). and the mapping pairs
> array of @MFT entry is empty, I think it's corrupted, it should cause
> mount failure.
>
> NAK
>
> Sorry but this patch is incorrect. It is perfectly valid to have an empty
> non-resident attribute. E.g. if you truncate a file to zero size this is
> exactly what you will get on-disk and when you then unmount and mount next
> time and try to access that file with your patch you will now get an -EIO
> error trying to access the file and you will not be able to write to the
> file nor truncate it as you will keep getting the i/o error.
Sorry, I can't reproduce the issue you described?

root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
total 5928
drwx------ 1 root root 4096 6월 24 23:01 .
drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
-rw------- 1 root root 6059409 9월 22 2020 foo
drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
root@linkinjeon-Z10PA-D8-Series:/mnt/test# truncate -s 0 foo
root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
total 8
drwx------ 1 root root 4096 6월 24 23:01 .
drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
-rw------- 1 root root 0 6월 24 23:11 foo
drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
root@linkinjeon-Z10PA-D8-Series:/mnt/test# cd ..
root@linkinjeon-Z10PA-D8-Series:/mnt# sudo umount /mnt/test
root@linkinjeon-Z10PA-D8-Series:/mnt# sudo mount -t ntfs /dev/sde2 /mnt/test/
root@linkinjeon-Z10PA-D8-Series:/mnt# cd /mnt/test/
root@linkinjeon-Z10PA-D8-Series:/mnt/test# cat foo
root@linkinjeon-Z10PA-D8-Series:/mnt/test# truncate -s 1048576 foo
root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
total 1032
drwx------ 1 root root 4096 6월 24 23:01 .
drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
-rw------- 1 root root 1048576 6월 24 23:12 foo
drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
root@linkinjeon-Z10PA-D8-Series:/mnt/test# echo "hello world" > foo
root@linkinjeon-Z10PA-D8-Series:/mnt/test# cat foo
hello world

>
> The correct solution is to use IS_ERR_OR_NULL() in places where an empty
> attribute is not acceptable. Such a case is for example when mounting the
> $MFT::$DATA::unnamed attribute cannot be empty which should then be
> addressed inside in fs/ntfs/inode.c::ntfs_read_inode_mount(). There may be
> more call sites to ntfs_mapping_pairs_decompress() which require similar
> treatment. Need to go through the code to see...
I think that it is needed everywhere that calls it. Am I missing something ?

I can not understand why the below code is needed in
ntfs_mapping_pairs_decompress().

/* If the mapping pairs array is valid but empty, nothing to do. */
if (!vcn && !*buf) {
return old_rl;
}

There is no description in patch. and this code is not in
ntfs_mapping_pairs_decompress() in ntfs-3g. Is there any case the
caller get NULL runlist pointer from ntfs_mapping_pairs_decompress()
in current ntfs code?

NTFS: Fix handling of valid but empty mapping pairs array in
fs/ntfs/runlist.c::ntfs_mapping_pairs_decompress().

Signed-off-by: Anton Altaparmakov <[email protected]>

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/ntfs/runlist.c?h=v5.19-rc3&id=2b0ada2b8e086c267dd116a39ad41ff0a717b665
>
> Best regards,
>
> Anton
>
>
> I haven't checked if this patch fix the problem. Xu, Can you check it ?
>
> diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
> index 97932fb5179c..31263fe0772f 100644
> --- a/fs/ntfs/runlist.c
> +++ b/fs/ntfs/runlist.c
> @@ -766,8 +766,11 @@ runlist_element
> *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
> return ERR_PTR(-EIO);
> }
> /* If the mapping pairs array is valid but empty, nothing to do. */
> - if (!vcn && !*buf)
> + if (!vcn && !*buf) {
> + if (!old_rl)
> + return ERR_PTR(-EIO);
> return old_rl;
> + }
> /* Current position in runlist array. */
> rlpos = 0;
> /* Allocate first page and set current runlist size to one page. */
>
>
> - Eric
>
>

2022-06-24 15:43:53

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

Hi,

> On 24 Jun 2022, at 15:37, Namjae Jeon <[email protected]> wrote:
> 2022-06-24 21:19 GMT+09:00, Anton Altaparmakov <[email protected]>:
>> Hi,
>>
>> On 24 Jun 2022, at 03:33, Namjae Jeon
>> <[email protected]<mailto:[email protected]>> wrote:
>>
>> 2022-06-24 2:08 GMT+09:00, Eric Biggers
>> <[email protected]<mailto:[email protected]>>:
>> On Thu, Jun 23, 2022 at 09:49:56AM +0000,
>> [email protected]<mailto:[email protected]> wrote:
>> From: xu xin <[email protected]<mailto:[email protected]>>
>>
>> As the bug description at
>> https://lore.kernel.org/lkml/[email protected]/
>> attckers can use this bug to crash the system.
>>
>> So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a
>> warning to the syslog and return instead until someone really solve
>> the problem.
>>
>> Cc: [email protected]
>> Reported-by: Zeal Robot <[email protected]>
>> Reported-by: [email protected]
>> Reviewed-by: Songyi Zhang <[email protected]>
>> Reviewed-by: Yang Yang <[email protected]>
>> Reviewed-by: Jiang Xuexin<[email protected]>
>> Reviewed-by: Zhang wenya<[email protected]>
>> Signed-off-by: xu xin <[email protected]>
>> ---
>>
>> Change for v2:
>> - Use ntfs_warning instead of WARN().
>> - Add the tag Cc: [email protected].
>> ---
>> fs/ntfs/aops.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
>> index 5f4fb6ca6f2e..84d68efb4ace 100644
>> --- a/fs/ntfs/aops.c
>> +++ b/fs/ntfs/aops.c
>> @@ -183,7 +183,12 @@ static int ntfs_read_block(struct page *page)
>> vol = ni->vol;
>>
>> /* $MFT/$DATA must have its complete runlist in memory at all times. */
>> - BUG_ON(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni));
>> + if (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) {
>> + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, "
>> + "and NInoAttr(ni) is null.");
>> + unlock_page(page);
>> + return -EINVAL;
>> + }
>>
>> A better warning message that doesn't rely on implementation details
>> (struct
>> field and macro names) would be "Runlist of $MFT/$DATA is not cached".
>> Also,
>> why does this situation happen in the first place? Is there a way to
>> prevent
>> this situation in the first place?
>>
>> ntfs_mapping_pairs_decompress() should return error pointer instead of
>> NULL.
>>
>> Callers is checking error value using IS_ERR(). and the mapping pairs
>> array of @MFT entry is empty, I think it's corrupted, it should cause
>> mount failure.
>>
>> NAK
>>
>> Sorry but this patch is incorrect. It is perfectly valid to have an empty
>> non-resident attribute. E.g. if you truncate a file to zero size this is
>> exactly what you will get on-disk and when you then unmount and mount next
>> time and try to access that file with your patch you will now get an -EIO
>> error trying to access the file and you will not be able to write to the
>> file nor truncate it as you will keep getting the i/o error.
> Sorry, I can't reproduce the issue you described?
>
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
> total 5928
> drwx------ 1 root root 4096 6월 24 23:01 .
> drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
> -rw------- 1 root root 6059409 9월 22 2020 foo
> drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# truncate -s 0 foo
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
> total 8
> drwx------ 1 root root 4096 6월 24 23:01 .
> drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
> -rw------- 1 root root 0 6월 24 23:11 foo
> drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# cd ..
> root@linkinjeon-Z10PA-D8-Series:/mnt# sudo umount /mnt/test
> root@linkinjeon-Z10PA-D8-Series:/mnt# sudo mount -t ntfs /dev/sde2 /mnt/test/
> root@linkinjeon-Z10PA-D8-Series:/mnt# cd /mnt/test/
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# cat foo
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# truncate -s 1048576 foo
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# ls -al
> total 1032
> drwx------ 1 root root 4096 6월 24 23:01 .
> drwxr-xr-x 7 root root 4096 5월 29 12:47 ..
> -rw------- 1 root root 1048576 6월 24 23:12 foo
> drwx------ 1 root root 0 6월 24 22:30 'System Volume Information'
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# echo "hello world" > foo
> root@linkinjeon-Z10PA-D8-Series:/mnt/test# cat foo
> hello world

It must be that we never attempt to map the runlist in this case. generic_file_read_iter() appears to never call into the file system because filemap_read() does:

if (unlikely(iocb->ki_pos >= i_size_read(inode)))
break;

I imagine that there are similar code paths in the VFS are why it currently works...

In fact for example has:

if (!rl && !allocated_size)
goto first_alloc;
rl = ntfs_mapping_pairs_decompress(vol, a, ni->runlist.rl);

So it skips calling ntfs_mapping_pairs_decompress() for an empty file but what if there is a runlist but allocated_size is incorrectly zero? We would now wipe that and lose it so it is not ideal to do it that way but it does make it work. Anyway it is because of things like that that at least some code paths cope with your patch.

But basically it is perfectly valid to have a non-resident attribute which has zero size and thus an empty runlist. This is not an error so you cannot start returning an error for this case!

Just because in this case the problem is avoided does not mean your patch is correct. NTFS also accesses attributes internally and if happens to attempt to map the runlist of an empty non-resident attribute it would now bail out with error instead of continuing correctly.

>> The correct solution is to use IS_ERR_OR_NULL() in places where an empty
>> attribute is not acceptable. Such a case is for example when mounting the
>> $MFT::$DATA::unnamed attribute cannot be empty which should then be
>> addressed inside in fs/ntfs/inode.c::ntfs_read_inode_mount(). There may be
>> more call sites to ntfs_mapping_pairs_decompress() which require similar
>> treatment. Need to go through the code to see...
> I think that it is needed everywhere that calls it. Am I missing something ?

It is not needed everywhere. It is only needed in code paths that require there be a runlist afterwards because the code paths assume there must be one. So for example for $MFT it by definition cannot have an empty runlist as we are already reading from the $MFT so it has an allocation so the runlist cannot be empty. But the fuzzed image that is showing the problem that you are trying to fix has such a corrupt volume that the runlist is in fact empty - note it is NOT corrupt but it is valid but empty. This then leads to the problem that we try to load the runlist for the $MFT and we do not check whether the result is empty because we assume it cannot be empty. Clearly when you have a corrupted image that assumption can be wrong so we need to check for both error and for it being empty, hence IS_ERR_OR_NULL(). But if we are accessing an attribute elsewhere in the driver and that happens to be empty then we want a NULL runlist to be obtained so we can then operate with the attribute. We do not want attempting to map the runlist to fail. The code can try to map the runlist for all non-resident attributes before working with them and if the attribute is zero length then the runlist is NULL until we write something to the attribute at which point it becomes non-NULL describing the allocated clusters.

Your patch violates the design of the code which is that empty attribute inodes have a NULL runlist and your patch will cause perfectly good attributes to be rejected if their runlist is attempted to be mapped when they are empty. You would then have to start checking everywhere in the code whether an attribute has zero allocated_size (or compressed_size if compressed and/or sparse) and if so you would need to then not attempt to map the runlist which would be ugly. Much better to have the code path streamline so it just returns an empty runlist...

It may be possible that the current OSS ntfs driver can indeed function with your patch but it still violates how the code is designed to work which is why I am not happy to take your patch.

> I can not understand why the below code is needed in
> ntfs_mapping_pairs_decompress().
>
> /* If the mapping pairs array is valid but empty, nothing to do. */
> if (!vcn && !*buf) {
> return old_rl;
> }
>
> There is no description in patch. and this code is not in
> ntfs_mapping_pairs_decompress() in ntfs-3g. Is there any case the
> caller get NULL runlist pointer from ntfs_mapping_pairs_decompress()
> in current ntfs code?

That fix was put in for a reason. Forgive me if I cannot remember after 17 years why that was... The fact that the patch says "fix handling" means that it was required to be put in to fix something. It is possible that afterwards the code changed so it no longer can get there in this case due to guards as pointed out above but fundamentally the function needs to be able to deal correctly with an empty runlist and if the caller expects to definitely get a runlist (as e.g. the $MFT loading code does at mount time) then it is for that caller to verify that the runlist is in fact not empty rather than to break the function instead to return errors for empty, non-resident attributes.

Best regards,

Anton

> NTFS: Fix handling of valid but empty mapping pairs array in
> fs/ntfs/runlist.c::ntfs_mapping_pairs_decompress().
>
> Signed-off-by: Anton Altaparmakov <[email protected]>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/ntfs/runlist.c?h=v5.19-rc3&id=2b0ada2b8e086c267dd116a39ad41ff0a717b665
>>
>> Best regards,
>>
>> Anton
>>
>>
>> I haven't checked if this patch fix the problem. Xu, Can you check it ?
>>
>> diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
>> index 97932fb5179c..31263fe0772f 100644
>> --- a/fs/ntfs/runlist.c
>> +++ b/fs/ntfs/runlist.c
>> @@ -766,8 +766,11 @@ runlist_element
>> *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
>> return ERR_PTR(-EIO);
>> }
>> /* If the mapping pairs array is valid but empty, nothing to do. */
>> - if (!vcn && !*buf)
>> + if (!vcn && !*buf) {
>> + if (!old_rl)
>> + return ERR_PTR(-EIO);
>> return old_rl;
>> + }
>> /* Current position in runlist array. */
>> rlpos = 0;
>> /* Allocate first page and set current runlist size to one page. */
>>
>>
>> - Eric

2022-07-05 07:49:32

by CGEL

[permalink] [raw]
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

On Fri, Jun 24, 2022 at 03:26:27PM +0000, Anton Altaparmakov wrote:
> Hi,
>
> > On 24 Jun 2022, at 15:37, Namjae Jeon <[email protected]> wrote:
> > 2022-06-24 21:19 GMT+09:00, Anton Altaparmakov <[email protected]>:
> >> Hi,
> >>
> >> On 24 Jun 2022, at 03:33, Namjae Jeon
> >> <[email protected]<mailto:[email protected]>> wrote:
> >>
> >> 2022-06-24 2:08 GMT+09:00, Eric Biggers
> >> <[email protected]<mailto:[email protected]>>:
> >> On Thu, Jun 23, 2022 at 09:49:56AM +0000,
> >> [email protected]<mailto:[email protected]> wrote:
> >> From: xu xin <[email protected]<mailto:[email protected]>>
> >>
> >> As the bug description at
> >> https://lore.kernel.org/lkml/[email protected]/
> >> attckers can use this bug to crash the system.
> >>
> >>
>
> >> The correct solution is to use IS_ERR_OR_NULL() in places where an empty
> >> attribute is not acceptable. Such a case is for example when mounting the
> >> $MFT::$DATA::unnamed attribute cannot be empty which should then be
> >> addressed inside in fs/ntfs/inode.c::ntfs_read_inode_mount(). There may be
> >> more call sites to ntfs_mapping_pairs_decompress() which require similar
> >> treatment. Need to go through the code to see...
> > I think that it is needed everywhere that calls it. Am I missing something ?
>
> It is not needed everywhere. It is only needed in code paths that require there be a runlist afterwards because the code paths assume there must be one. So for example for $MFT it by definition cannot have an empty runlist as we are already reading from the $MFT so it has an allocation so the runlist cannot be empty. But the fuzzed image that is showing the problem that you are trying to fix has such a corrupt volume that the runlist is in fact empty - note it is NOT corrupt but it is valid but empty. This then leads to the problem that we try to load the runlist for the $MFT and we do not check whether the result is empty because we assume it cannot be empty. Clearly when you have a corrupted image that assumption can be wrong so we need to check for both error and for it being empty, hence IS_ERR_OR_NULL(). But if we are accessing an attribute elsewhere in the driver and that happens to be empty then we want a NULL runlist to be obtained so we can then operate with the attribute. We do not want attempting to map the runlist to fail. The code can try to map the runlist for all non-resident attributes before working with them and if the attribute is zero length then the runlist is NULL until we write something to the attribute at which point it becomes non-NULL describing the allocated clusters.


So to fix the current bug, how about this patch?

--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -183,7 +183,14 @@ static int ntfs_read_block(struct page *page)
vol = ni->vol;

/* $MFT/$DATA must have its complete runlist in memory at all
* times. */
- BUG_ON(!ni->runlist.rl && !ni->mft_no &&
!NInoAttr(ni));
+ if (IS_ERR_OR_NULL(ni->runlist.rl) && !ni->mft_no
&& !NInoAttr(ni)) {
+ ntfs_error(vol->sb, "Runlist of $MFT/$DATA is not cached. "
+ "$MFT is corrupt.");
+ unlock_page(page);
+ if (IS_ERR(ni->runlist.rl))
+ return PTR_ERR(ni->runlist.rl);
+ return -EFAULT;
+ }
--

Thanks
xu xin

>
> Your patch violates the design of the code which is that empty attribute inodes have a NULL runlist and your patch will cause perfectly good attributes to be rejected if their runlist is attempted to be mapped when they are empty. You would then have to start checking everywhere in the code whether an attribute has zero allocated_size (or compressed_size if compressed and/or sparse) and if so you would need to then not attempt to map the runlist which would be ugly. Much better to have the code path streamline so it just returns an empty runlist...
>
> It may be possible that the current OSS ntfs driver can indeed function with your patch but it still violates how the code is designed to work which is why I am not happy to take your patch.
>
> > I can not understand why the below code is needed in
> > ntfs_mapping_pairs_decompress().
> >
> > /* If the mapping pairs array is valid but empty, nothing to do. */
> > if (!vcn && !*buf) {
> > return old_rl;
> > }
> >
> >> +++ b/fs/ntfs/runlist.c
> >> @@ -766,8 +766,11 @@ runlist_element
> >> *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
> >> return ERR_PTR(-EIO);
> >> }
> >> /* If the mapping pairs array is valid but empty, nothing to do. */
> >> - if (!vcn && !*buf)
> >> + if (!vcn && !*buf) {
> >> + if (!old_rl)
> >> + return ERR_PTR(-EIO);
> >> return old_rl;
> >> + }
> >> /* Current position in runlist array. */
> >> rlpos = 0;
> >> /* Allocate first page and set current runlist size to one page. */
> >>
> >>
> >> - Eric