ext4: oops in ext4_finish_bio() after enabling encryption on a mounted fs

Hi,

I'd like to report a bug in the ext4 code. I confirm that it happens with the latest stable kernel (7.2.7), but mainline (7.3-rc4) does not seem to be affected.

The problem is very easy to reproduce:

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt

2) Create an empty directory and encrypt it (fscrypt encrypt /foo or whatever).
3) Write some data to it (head -c head -c 10M /dev/urandom > /foo/file.bin)
4) sync

[   42.962615] BUG: kernel NULL pointer dereference, address: 0000000000000028
[   42.965440] #PF: supervisor read access in kernel mode
[   42.967480] #PF: error_code(0x0000) - not-present page
[   42.969475] PGD 0 P4D 0 
[   42.970460] Oops: Oops: 0000 [#1] SMP NOPTI
[   42.972141] CPU: 0 UID: 0 PID: 12 Comm: kworker/u16:0 Not tainted 7.2.7-vanilla #2 PREEMPT(lazy) 
[   42.975502] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   42.977650] Workqueue: ext4-rsv-conversion ext4_end_io_rsv_work
[   42.978499] RIP: 0010:ext4_finish_bio+0xf8/0x400
[   42.979452] Code: 38 00 00 00 00 48 8b 44 24 08 4c 01 f8 48 89 04 24 48 83 7b 18 00 0f 84 d7 02 00 00 41 0f b6 7d 1a 40 84 ff 0f 85 7d 02 00 00 <4c> 8b 63 28 45 31 f6 49 8d 44 24 5c 48 89 c7 48 89 44 24 20 e8 6f
[   42.982351] RSP: 0018:ffffd0290006bd40 EFLAGS: 00010246
[   42.983090] RAX: 0000000000001000 RBX: 0000000000000000 RCX: 000fffffc0000201
[   42.984204] RDX: fffff64a40226f00 RSI: fffff64a400afcc0 RDI: 0000000000000000
[   42.985203] RBP: 0000000000001000 R08: 0000000000001000 R09: ffffffff8a49cb8a
[   42.986203] R10: 0000000000001fff R11: ffff8b4f81926200 R12: ffff8b4f8521b540
[   42.987213] R13: ffff8b4f81927f00 R14: 0000000000000001 R15: 0000000000000000
[   42.988351] FS:  0000000000000000(0000) GS:ffff8b507105e000(0000) knlGS:0000000000000000
[   42.989478] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.990292] CR2: 0000000000000028 CR3: 000000000bcb9000 CR4: 0000000000350ef0
[   42.991291] Call Trace:
[   42.991709]  <TASK>
[   42.992042]  ext4_release_io_end+0x55/0x120
[   42.992650]  ext4_end_io_end+0x4c/0xe0
[   42.993194]  ext4_end_io_rsv_work+0xaa/0x100
[   42.993807]  process_one_work+0x19e/0x370
[   42.994382]  worker_thread+0x1a6/0x310
[   42.994925]  ? __pfx_worker_thread+0x10/0x10
[   42.995806]  kthread+0xe4/0x120
[   42.996313]  ? __pfx_kthread+0x10/0x10
[   42.996856]  ret_from_fork+0x2b1/0x340
[   42.997397]  ? __pfx_kthread+0x10/0x10
[   42.997941]  ret_from_fork_asm+0x1a/0x30
[   42.998505]  </TASK>

The problem does not happen if you unmount the filesystem after setting the encryption feature, and then mount it again.

The cause seems to be that encryption does not support large folios, but after commit 709f0f1f1bf5c ("ext4: add checks for large folio incompatibilities when BS > PS") this is only checked at mount time, so enabling encryption on a mounted fs bypasses this check.

This solves the crash for me, but I suppose that it also needs an additional check to decide whether encryption can be enabled on a filesystem (i.e. if sb->s_blocksize > PAGE_SIZE).

fs/ext4/inode.c +2 -1

@@ -5287,7 +5287,8 @@ void ext4_set_inode_mapping_order(struct inode *inode)
 	if (!min_order && !S_ISREG(inode->i_mode))
 		return;
 
-	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) ||
+	    ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT))
 		max_order = min_order;
 
 	mapping_set_folio_order_range(inode->i_mapping, min_order, max_order);

Regards,

Berto

Quoted 1 line

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt

Is it valid to enable encrypt at mount time?

Quoted 2 lines · Baokun Li

Quoted 1 line

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt

Is it valid to enable encrypt at mount time?

Good question, I always understood that it was allowed and tune2fs certainly doesn't forbid it (compare with casefold):

https://github.com/tytso/e2fsprogs/blob/v1.47.4/misc/tune2fs.c#L1599

Berto

Quoted 8 lines · Alberto Garcia

Quoted 2 lines · Baokun Li

Quoted 1 line

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt

Is it valid to enable encrypt at mount time?

Good question, I always understood that it was allowed and tune2fs certainly doesn't forbid it (compare with casefold):

https://github.com/tytso/e2fsprogs/blob/v1.47.4/misc/tune2fs.c#L1599

Quoted 1 line

Berto

If enabling encryption on a mounted ext4 fs is allowed, I think the current change is fine.

Ted, Eric - would either of you know the details here?

Quoted 14 lines · Baokun Li

Quoted 9 lines · Alberto Garcia

Quoted 2 lines · Baokun Li

Quoted 1 line

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt

Is it valid to enable encrypt at mount time?

Good question, I always understood that it was allowed and tune2fs certainly doesn't forbid it (compare with casefold):

https://github.com/tytso/e2fsprogs/blob/v1.47.4/misc/tune2fs.c#L1599

Berto

If enabling encryption on a mounted ext4 fs is allowed, I think the current change is fine.

Quoted 1 line

Ted, Eric - would either of you know the details here?

Yes, it is allowed.

The proposed patch looks good, even though the problem seems to be gone on mainline already due to the removal of the code path that used the non-large-folio-compatible function fscrypt_encrypt_pagecache_blocks().

There can be another patch that removes both that check and the mount-time check, if they're truly no longer needed (I don't know of any reason why they would be, but it needs to be properly tested).

The proposed patch looks good

Quoted 1 line · Eric Biggers

The proposed patch looks good

As I said, I think we need an additional check to detect if encryption is set on a mounted filesystem that has blocksize > pagesize.

Such a filesystem cannot be mounted (ext4_check_large_folio refuses), but if you do it after the fs is mounted you can crash the kernel using the same method.

# mkfs.ext4 -b 16384 /dev/vdb 
# mount /dev/vdb /mnt/
# fscrypt setup /mnt
# tune2fs -O encrypt /dev/vdb 
# mkdir /mnt/foo
# fscrypt encrypt /mnt/foo/

The other patch alone does not prevent this.

With this additional change, FS_IOC_SET_ENCRYPTION_POLICY returns

(quoted diff) +0 -1

-EOPNOTSUPP, so tune2fs succeeds, but 'fscrypt encrypt' fails:
 
   [ERROR] fscrypt encrypt: encryption not enabled on filesystem /mnt (/dev/vdb).

fs/ext4/crypto.c +3 -0

@@ -156,6 +156,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))
 		return -EOPNOTSUPP;
 
+	if (inode->i_sb->s_blocksize > PAGE_SIZE)
+		return -EOPNOTSUPP;
+
 	res = ext4_convert_inline_data(inode);
 	if (res)
 		return res;

That prevents the crash, but the filesystem cannot be remounted:

   kernel: EXT4-fs (vdb): bs(16384) > ps(4096) unsupported for encrypt

Would this be enough or is there anything else to take into account?

* And maybe a check in __ext4_iget() to prevent existing encrypted inodes from being loaded when fs_block_size > PAGE_SIZE. This would be needed for fuzzing robustness, where a fuzzer generates a filesystem that has encrypted files without the encrypt feature flag.

Quoted 4 lines · Eric Biggers

* And maybe a check in __ext4_iget() to prevent existing encrypted inodes from being loaded when fs_block_size > PAGE_SIZE. This would be needed for fuzzing robustness, where a fuzzer generates a filesystem that has encrypted files without the encrypt feature flag.

Does the kernel reject encrypted files on a fs without the encrypt flag? I'm asking in general, regardless of the block size. Should it?

On a filesystem without the encrypt flag, ext4 rejects creating new encrypted directories, but it doesn't reject accessing existing encrypted directories.

It *should* reject accessing existing encrypted directories. [...] The only caveat is that, technically, if an existing encrypted directory is using the old policy version (which before v5.4 was the only option), it can be unlocked and accessed without executing any of the encryption ioctls. So in theory someone could be depending on that on a filesystem without the encrypt flag. I think it's unlikely at this point, though.

Quoted 11 lines · Eric Biggers

On a filesystem without the encrypt flag, ext4 rejects creating new encrypted directories, but it doesn't reject accessing existing encrypted directories.

It *should* reject accessing existing encrypted directories. [...] The only caveat is that, technically, if an existing encrypted directory is using the old policy version (which before v5.4 was the only option), it can be unlocked and accessed without executing any of the encryption ioctls. So in theory someone could be depending on that on a filesystem without the encrypt flag. I think it's unlikely at this point, though.

One problem that I see is that e2fsck does not seem to detect such filesystems. Wouldn't it make sense to fix it there first? (I suppose by setting the encrypt feature if an encrypted inode is found)

I'd like to report a bug in the ext4 code. I confirm that it happens with the latest stable kernel (7.2.7), but mainline (7.3-rc4) does not seem to be affected.

The problem is very easy to reproduce:

1) Enable encryption on an ext4 filesystem with tune2fs -O encrypt
2) Create an empty directory and encrypt it (fscrypt encrypt /foo or whatever).
3) Write some data to it (head -c head -c 10M /dev/urandom > /foo/file.bin)
4) sync

[   42.962615] BUG: kernel NULL pointer dereference, address: 0000000000000028
[   42.965440] #PF: supervisor read access in kernel mode
[   42.967480] #PF: error_code(0x0000) - not-present page
[   42.969475] PGD 0 P4D 0 
[   42.970460] Oops: Oops: 0000 [#1] SMP NOPTI
[   42.972141] CPU: 0 UID: 0 PID: 12 Comm: kworker/u16:0 Not tainted 7.2.7-vanilla #2 PREEMPT(lazy) 
[   42.975502] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   42.977650] Workqueue: ext4-rsv-conversion ext4_end_io_rsv_work
[   42.978499] RIP: 0010:ext4_finish_bio+0xf8/0x400
[   42.979452] Code: 38 00 00 00 00 48 8b 44 24 08 4c 01 f8 48 89 04 24 48 83 7b 18 00 0f 84 d7 02 00 00 41 0f b6 7d 1a 40 84 ff 0f 85 7d 02 00 00 <4c> 8b 63 28 45 31 f6 49 8d 44 24 5c 48 89 c7 48 89 44 24 20 e8 6f
[   42.982351] RSP: 0018:ffffd0290006bd40 EFLAGS: 00010246
[   42.983090] RAX: 0000000000001000 RBX: 0000000000000000 RCX: 000fffffc0000201
[   42.984204] RDX: fffff64a40226f00 RSI: fffff64a400afcc0 RDI: 0000000000000000
[   42.985203] RBP: 0000000000001000 R08: 0000000000001000 R09: ffffffff8a49cb8a
[   42.986203] R10: 0000000000001fff R11: ffff8b4f81926200 R12: ffff8b4f8521b540
[   42.987213] R13: ffff8b4f81927f00 R14: 0000000000000001 R15: 0000000000000000
[   42.988351] FS:  0000000000000000(0000) GS:ffff8b507105e000(0000) knlGS:0000000000000000
[   42.989478] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.990292] CR2: 0000000000000028 CR3: 000000000bcb9000 CR4: 0000000000350ef0
[   42.991291] Call Trace:
[   42.991709]  <TASK>
[   42.992042]  ext4_release_io_end+0x55/0x120
[   42.992650]  ext4_end_io_end+0x4c/0xe0
[   42.993194]  ext4_end_io_rsv_work+0xaa/0x100
[   42.993807]  process_one_work+0x19e/0x370
[   42.994382]  worker_thread+0x1a6/0x310
[   42.994925]  ? __pfx_worker_thread+0x10/0x10
[   42.995806]  kthread+0xe4/0x120
[   42.996313]  ? __pfx_kthread+0x10/0x10
[   42.996856]  ret_from_fork+0x2b1/0x340
[   42.997397]  ? __pfx_kthread+0x10/0x10
[   42.997941]  ret_from_fork_asm+0x1a/0x30
[   42.998505]  </TASK>

The problem does not happen if you unmount the filesystem after setting the encryption feature, and then mount it again.

The cause seems to be that encryption does not support large folios, but after commit 709f0f1f1bf5c ("ext4: add checks for large folio incompatibilities when BS > PS") this is only checked at mount time, so enabling encryption on a mounted fs bypasses this check.

This solves the crash for me, but I suppose that it also needs an additional check to decide whether encryption can be enabled on a filesystem (i.e. if sb->s_blocksize > PAGE_SIZE).

fs/ext4/inode.c +2 -1

@@ -5287,7 +5287,8 @@ void ext4_set_inode_mapping_order(struct inode *inode)
 	if (!min_order && !S_ISREG(inode->i_mode))
 		return;
 
-	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) ||
+	    ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT))
 		max_order = min_order;
 
 	mapping_set_folio_order_range(inode->i_mapping, min_order, max_order);

Regards,

Berto

Quoted 3 lines

I'd like to report a bug in the ext4 code. I confirm that it happens with the latest stable kernel (7.2.7), but mainline (7.3-rc4) does not seem to be affected.

Hum, so 709f0f1f1bf5c was merged to 6.19. So did you have a look what has fixed the problem in 7.3-rc4? Because I'm not aware of any ext4 change that should fix this.