Fix the bug reported at:
https://syzkaller.appspot.com/bug?id=79d5768e9bfe362911ac1a5057a36fc6b5c30002
Darrick J. Wong proposed a similar patch to address the same bug at:
https://lore.kernel.org/linux-ext4/[email protected]/
I think my version of the patch is better. It clearly indicates that
lower out of bounds requests are ignored. The high key should be greater
than the first data block for the ext4_getfsmap_datadev() handler,
otherwise there's no data to return, thus we exit early and ignore the
request. Darrick indirectly implied the same thing, but missed the case
where the high_key->fmr_phisical is equal to the first data block.
After the fix you'll find another patch that consolidates the validation
of the user provided data. Instead of having the checks scattered among
the fsmap representations, gather the code in a single method and do the
checks directly on the data received from user.
Similar patch can be done for xfs fsmap, but I'll wait for some
feedback first.
Tested the changes with kvm-xfstests: ext4/{027, 028, 029}, all passed,
output below.
Cheers,
ta
-------------------- Summary report
KERNEL: kernel 6.2.0-rc8-xfstests-00003-gc34cc283e325 #13 SMP PREEMPT_DYNAMIC Wed Feb 22 12:35:39 UTC 2023 x86_64
CMDLINE: ext4/027
CPUS: 2
MEM: 1975.3
ext4/4k: 1 tests, 1 seconds
ext4/027 Pass 1s
ext4/1k: 1 tests, 1 seconds
ext4/027 Pass 1s
ext4/ext3: 1 tests, 2 seconds
ext4/027 Pass 1s
ext4/encrypt: 1 tests, 1 seconds
ext4/027 Pass 0s
ext4/nojournal: 1 tests, 1 seconds
ext4/027 Pass 1s
ext4/ext3conv: 1 tests, 1 seconds
ext4/027 Pass 0s
ext4/adv: 1 tests, 1 seconds
ext4/027 Pass 1s
ext4/dioread_nolock: 1 tests, 1 seconds
ext4/027 Pass 1s
ext4/data_journal: 1 tests, 1 seconds
ext4/027 Pass 0s
ext4/bigalloc: 1 tests, 1 seconds
ext4/027 Pass 0s
ext4/bigalloc_1k: 1 tests, 1 seconds
ext4/027 Pass 0s
Totals: 11 tests, 0 skipped, 0 failures, 0 errors, 6s
FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: all
FSTESTSET: ext4/027
FSTESTOPT: aex
[ 59.553199] ACPI: PM: Preparing to enter system sleep state S5
[ 59.557660] reboot: Power down
-------------------- Summary report
KERNEL: kernel 6.2.0-rc8-xfstests-00003-gc34cc283e325 #13 SMP PREEMPT_DYNAMIC Wed Feb 22 12:35:39 UTC 2023 x86_64
CMDLINE: ext4/028
CPUS: 2
MEM: 1975.31
ext4/4k: 1 tests, 1 seconds
ext4/028 Pass 1s
ext4/1k: 1 tests, 3 seconds
ext4/028 Pass 3s
ext4/ext3: 1 tests, 1 skipped, 1 seconds
ext4/028 Skipped 1s
ext4/encrypt: 0 tests, 0 seconds
ext4/nojournal: 1 tests, 4 seconds
ext4/028 Pass 4s
ext4/ext3conv: 1 tests, 5 seconds
ext4/028 Pass 4s
ext4/adv: 1 tests, 4 seconds
ext4/028 Pass 4s
ext4/dioread_nolock: 1 tests, 1 seconds
ext4/028 Pass 1s
ext4/data_journal: 1 tests, 1 seconds
ext4/028 Pass 1s
ext4/bigalloc: 1 tests, 5 seconds
ext4/028 Pass 5s
ext4/bigalloc_1k: 1 tests, 3 seconds
ext4/028 Pass 2s
Totals: 10 tests, 1 skipped, 0 failures, 0 errors, 26s
FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: all
FSTESTSET: ext4/028
FSTESTOPT: aex
[ 76.557142] EXT4-fs (vdg): unmounting filesystem 3149a29d-9b44-4c17-82a6-c86addd7f1bb.
[ 76.592295] ACPI: PM: Preparing to enter system sleep state S5
[ 76.597019] reboot: Power down
-------------------- Summary report
KERNEL: kernel 6.2.0-rc8-xfstests-00003-gc34cc283e325 #13 SMP PREEMPT_DYNAMIC Wed Feb 22 12:35:39 UTC 2023 x86_64
CMDLINE: -c logdev ext4/029
CPUS: 2
MEM: 1975.31
ext4/logdev: 1 tests, 1 seconds
ext4/029 Pass 1s
Totals: 1 tests, 0 skipped, 0 failures, 0 errors, 1s
FSTESTVER: blktests 4e07b0c (Fri, 15 Jul 2022 14:40:03 +0900)
FSTESTVER: fio fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5 (Sun, 6 Feb 2022 10:59:13 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota v4.05-43-gd2256ac (Fri, 17 Sep 2021 14:04:16 +0200)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v5.19.0 (Fri, 12 Aug 2022 13:45:01 -0500)
FSTESTVER: xfstests v2022.08.21-8-g289f50f8 (Sun, 21 Aug 2022 15:21:34 -0400)
FSTESTVER: xfstests-bld bb566bcf (Wed, 24 Aug 2022 23:07:24 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: logdev
FSTESTSET: ext4/029
FSTESTOPT: aex
[ 8.217384] reboot: Power down
Tudor Ambarus (3):
ext4: fsmap: Fix crash caused by poor key validation
ext4: fsmap: Consolidate fsmap_head checks
ext4: fsmap: Remove duplicated initialization
fs/ext4/fsmap.c | 56 +++++++++++++++++++++++++++++++++++--------------
fs/ext4/fsmap.h | 3 +++
fs/ext4/ioctl.c | 17 +++------------
3 files changed, 46 insertions(+), 30 deletions(-)
--
2.39.2.637.g21b0678d19-goog
The following crash was seen on a 1k-block ext4 filesystem when the user
passed a fmr_physical of value zero for the high key:
kernel BUG at fs/ext4/ext4.h:3354!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 422 Comm: syz-executor339 Not tainted 5.15.74-syzkaller-00001-g4ec71a9ec769 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
RIP: 0010:ext4_get_group_info fs/ext4/ext4.h:3354 [inline]
RIP: 0010:ext4_mb_load_buddy_gfp+0xe9d/0xeb0 fs/ext4/mballoc.c:1498
Code: 99 ed c1 ff e9 47 f4 ff ff e8 5f a0 7f ff 48 c7 c7 c0 c3 a9 86 48 89 de 4c 89 ea e8 ad 8e 92 00 e9 a1 f2 ff ff e8 43 a0 7f ff <0f> 0b e8 3c a0 7f ff 0f 0b e8 35 a0 7f ff 0f 0b 0f 1f 00 55 48 89
RSP: 0018:ffffc9000044f320 EFLAGS: 00010293
RAX: ffffffff81f1f14d RBX: 0000000000000001 RCX: ffff888106194f00
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000001
RBP: ffffc9000044f3b0 R08: ffffffff81f1e3a4 R09: ffffc9000044f440
R10: fffff52000089e8f R11: 1ffff92000089e88 R12: ffff888100a553c8
R13: dffffc0000000000 R14: 0000000000000001 R15: ffff888105f9a000
FS: 000055555675b300(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055c4f7cde3e8 CR3: 000000011c064000 CR4: 00000000003506b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ext4_mb_load_buddy fs/ext4/mballoc.c:1620 [inline]
ext4_mballoc_query_range+0xb8/0x7b0 fs/ext4/mballoc.c:6560
ext4_getfsmap_datadev+0x1c8a/0x28c0 fs/ext4/fsmap.c:537
ext4_getfsmap+0xcff/0x1040 fs/ext4/fsmap.c:708
ext4_ioc_getfsmap fs/ext4/ioctl.c:660 [inline]
__ext4_ioctl fs/ext4/ioctl.c:862 [inline]
ext4_ioctl+0x3020/0x50e0 fs/ext4/ioctl.c:1276
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:874 [inline]
__se_sys_ioctl+0x115/0x190 fs/ioctl.c:860
__x64_sys_ioctl+0x7b/0x90 fs/ioctl.c:860
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x61/0xcb
RIP: 0033:0x7f8d2372e3e9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffcae7c2578 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000000f4240 RCX: 00007f8d2372e3e9
RDX: 0000000020000200 RSI: 00000000c0c0583b RDI: 0000000000000004
RBP: 0000000000000000 R08: 000000000000000d R09: 000000000000000d
R10: 00000000000003f1 R11: 0000000000000246 R12: 00007f8d236ed5c0
R13: 00007ffcae7c25a0 R14: 00007ffcae7c258c R15: 00007ffcae7c2590
The crash is due to insufficient key validation.
When high_key->fmr_physical is zero for a 1k-block ext4 filesystem we
encounter an underflow in the second call to
ext4_get_group_no_and_offset():
blocknr = blocknr - le32_to_cpu(es->s_first_data_block);
blocknr comes with value zero (high_key->fmr_physical is zero) and
the first_data_block is one, thus blocknr becomes -1. Since blocknr is
of type ext4_fsblk_t (unsigned long long) the value is all ones. Due to
the high values for blocknr and offset we hit the BUG_ON in
ext4_get_group_info() as the groupnr exceeds EXT4_SB(sb)->s_groups_count).
Fix it by returning early when keys[1].fmr_physical is less than or equal
to the first data block, as there's no data to return. This replicates the
behavior of returning zero, like in keys[0].fmr_physical >= eofs. In both
cases -EINVAL could be returned but for some reason the author decided to
just ignore out of bounds requests.
One may notice that the check introduced addresses just the
ext4_getfsmap_datadev() handler. This is intentional, because for the
ext4_getfsmap_logdev() handler, the value of the high key passed by the
user is ignored and instead the code uses:
memset(&info->gfi_high, 0xFF, sizeof(info->gfi_high));
Cc: [email protected]
Fixes: 0c9ec4beecac ("ext4: support GETFSMAP ioctls")
Link: https://syzkaller.appspot.com/bug?id=79d5768e9bfe362911ac1a5057a36fc6b5c30002
Reported-by: [email protected]
Signed-off-by: Tudor Ambarus <[email protected]>
---
fs/ext4/fsmap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index 4493ef0c715e..b5289378a761 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -479,6 +479,8 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
int error = 0;
bofs = le32_to_cpu(sbi->s_es->s_first_data_block);
+ if (keys[1].fmr_physical <= bofs)
+ return 0;
eofs = ext4_blocks_count(sbi->s_es);
if (keys[0].fmr_physical >= eofs)
return 0;
--
2.39.2.637.g21b0678d19-goog
All members of struct ext4_fsmap_head were already initialized with zero
in the caller, ext4_ioc_getfsmap(), remove duplicated initialization.
Signed-off-by: Tudor Ambarus <[email protected]>
---
fs/ext4/fsmap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index a27d9f0967b7..348eaffaa4d8 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -668,8 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
int i;
int error = 0;
- head->fmh_entries = 0;
-
/* Set up our device handlers. */
memset(handlers, 0, sizeof(handlers));
handlers[0].gfd_dev = new_encode_dev(sb->s_bdev->bd_dev);
--
2.39.2.637.g21b0678d19-goog
Sanity checks should be done the soonest possible to avoid superfluous
computations when user provides wrong data. Gather all the checks on
user provided data in a single method and call it immediately after
copying the data from user.
Signed-off-by: Tudor Ambarus <[email protected]>
---
fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++-------------
fs/ext4/fsmap.h | 3 +++
fs/ext4/ioctl.c | 17 +++-------------
3 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
index b5289378a761..a27d9f0967b7 100644
--- a/fs/ext4/fsmap.c
+++ b/fs/ext4/fsmap.c
@@ -9,6 +9,7 @@
#include "fsmap.h"
#include "mballoc.h"
#include <linux/sort.h>
+#include <linux/string.h>
#include <linux/list_sort.h>
#include <trace/events/ext4.h>
@@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
/* Do we recognize the device? */
static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
- struct ext4_fsmap *fm)
+ const struct fsmap *fm)
{
if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
@@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
}
/* Ensure that the low key is less than the high key. */
-static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
- struct ext4_fsmap *high_key)
+static bool ext4_getfsmap_check_keys(const struct fsmap *low_key,
+ const struct fsmap *high_key)
{
+ u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
+
if (low_key->fmr_device > high_key->fmr_device)
return false;
if (low_key->fmr_device < high_key->fmr_device)
return true;
- if (low_key->fmr_physical > high_key->fmr_physical)
+ if (l_fmr_phys > high_key->fmr_physical)
return false;
- if (low_key->fmr_physical < high_key->fmr_physical)
+ if (l_fmr_phys < high_key->fmr_physical)
return true;
if (low_key->fmr_owner > high_key->fmr_owner)
@@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
return false;
}
+int ext4_fsmap_check_head(struct super_block *sb,
+ const struct fsmap_head *head)
+{
+ const struct fsmap *l = &head->fmh_keys[0];
+ const struct fsmap *h = &head->fmh_keys[1];
+
+ if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) ||
+ memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) ||
+ memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved)))
+ return -EINVAL;
+ /*
+ * ext4 doesn't report file extents at all, so the only valid
+ * file offsets are the magic ones (all zeroes or all ones).
+ */
+ if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL))
+ return -EINVAL;
+
+ if (head->fmh_iflags & ~FMH_IF_VALID)
+ return -EINVAL;
+
+ if (!ext4_getfsmap_is_valid_device(sb, l) ||
+ !ext4_getfsmap_is_valid_device(sb, h))
+ return -EINVAL;
+
+ if (!ext4_getfsmap_check_keys(l, h))
+ return -EINVAL;
+
+ return 0;
+}
+
#define EXT4_GETFSMAP_DEVS 2
/*
* Get filesystem's extents as described in head, and format for
@@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
int i;
int error = 0;
- if (head->fmh_iflags & ~FMH_IF_VALID)
- return -EINVAL;
- if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
- !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
- return -EINVAL;
-
head->fmh_entries = 0;
/* Set up our device handlers. */
@@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
dkeys[0].fmr_length = 0;
memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap));
- if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1]))
- return -EINVAL;
-
info.gfi_next_fsblk = head->fmh_keys[0].fmr_physical +
head->fmh_keys[0].fmr_length;
info.gfi_formatter = formatter;
diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h
index ac642be2302e..8325258def7b 100644
--- a/fs/ext4/fsmap.h
+++ b/fs/ext4/fsmap.h
@@ -8,6 +8,7 @@
#define __EXT4_FSMAP_H__
struct fsmap;
+struct fsmap_head;
/* internal fsmap representation */
struct ext4_fsmap {
@@ -32,6 +33,8 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest,
struct ext4_fsmap *src);
void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest,
struct fsmap *src);
+int ext4_fsmap_check_head(struct super_block *sb,
+ const struct fsmap_head *head);
/* fsmap to userspace formatter - copy to user & advance pointer */
typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 8067ccda34e4..eb0ecb012e6a 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -874,20 +874,9 @@ static int ext4_ioc_getfsmap(struct super_block *sb,
if (copy_from_user(&head, arg, sizeof(struct fsmap_head)))
return -EFAULT;
- if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
- memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
- sizeof(head.fmh_keys[0].fmr_reserved)) ||
- memchr_inv(head.fmh_keys[1].fmr_reserved, 0,
- sizeof(head.fmh_keys[1].fmr_reserved)))
- return -EINVAL;
- /*
- * ext4 doesn't report file extents at all, so the only valid
- * file offsets are the magic ones (all zeroes or all ones).
- */
- if (head.fmh_keys[0].fmr_offset ||
- (head.fmh_keys[1].fmr_offset != 0 &&
- head.fmh_keys[1].fmr_offset != -1ULL))
- return -EINVAL;
+ error = ext4_fsmap_check_head(sb, &head);
+ if (error)
+ return error;
xhead.fmh_iflags = head.fmh_iflags;
xhead.fmh_count = head.fmh_count;
--
2.39.2.637.g21b0678d19-goog
On Wed, Feb 22, 2023 at 01:12:10PM +0000, Tudor Ambarus wrote:
> Sanity checks should be done the soonest possible to avoid superfluous
> computations when user provides wrong data. Gather all the checks on
> user provided data in a single method and call it immediately after
> copying the data from user.
This patch changes the validation criteria, moves chunks of code around,
and constifies parameters all at once. And all you say here is that
you're moving validation code up in the sequence!
Also, how does moving callsites around improve things? Do the fstests
still pass?
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++-------------
> fs/ext4/fsmap.h | 3 +++
> fs/ext4/ioctl.c | 17 +++-------------
> 3 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
> index b5289378a761..a27d9f0967b7 100644
> --- a/fs/ext4/fsmap.c
> +++ b/fs/ext4/fsmap.c
> @@ -9,6 +9,7 @@
> #include "fsmap.h"
> #include "mballoc.h"
> #include <linux/sort.h>
> +#include <linux/string.h>
> #include <linux/list_sort.h>
> #include <trace/events/ext4.h>
>
> @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
>
> /* Do we recognize the device? */
> static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
> - struct ext4_fsmap *fm)
> + const struct fsmap *fm)
> {
> if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
> fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
> @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
> }
>
> /* Ensure that the low key is less than the high key. */
> -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
> - struct ext4_fsmap *high_key)
> +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key,
> + const struct fsmap *high_key)
> {
> + u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
> +
> if (low_key->fmr_device > high_key->fmr_device)
> return false;
> if (low_key->fmr_device < high_key->fmr_device)
> return true;
>
> - if (low_key->fmr_physical > high_key->fmr_physical)
> + if (l_fmr_phys > high_key->fmr_physical)
> return false;
> - if (low_key->fmr_physical < high_key->fmr_physical)
> + if (l_fmr_phys < high_key->fmr_physical)
Why are you changing the comparison here?
> return true;
>
> if (low_key->fmr_owner > high_key->fmr_owner)
> @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
> return false;
> }
>
> +int ext4_fsmap_check_head(struct super_block *sb,
> + const struct fsmap_head *head)
> +{
> + const struct fsmap *l = &head->fmh_keys[0];
> + const struct fsmap *h = &head->fmh_keys[1];
> +
> + if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) ||
> + memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) ||
> + memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved)))
> + return -EINVAL;
> + /*
> + * ext4 doesn't report file extents at all, so the only valid
> + * file offsets are the magic ones (all zeroes or all ones).
> + */
> + if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL))
> + return -EINVAL;
> +
> + if (head->fmh_iflags & ~FMH_IF_VALID)
> + return -EINVAL;
> +
> + if (!ext4_getfsmap_is_valid_device(sb, l) ||
> + !ext4_getfsmap_is_valid_device(sb, h))
> + return -EINVAL;
> +
> + if (!ext4_getfsmap_check_keys(l, h))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> #define EXT4_GETFSMAP_DEVS 2
> /*
> * Get filesystem's extents as described in head, and format for
> @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
> int i;
> int error = 0;
>
> - if (head->fmh_iflags & ~FMH_IF_VALID)
> - return -EINVAL;
> - if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
> - !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
> - return -EINVAL;
> -
> head->fmh_entries = 0;
>
> /* Set up our device handlers. */
> @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
> dkeys[0].fmr_length = 0;
> memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap));
>
> - if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1]))
> - return -EINVAL;
And why is it ok to turn validation of dkeys[0] vs. fmh_keys[1] into a
validation of fmh_keys[0..1] ? I guess that's why check_keys now adds
the low key physical offset and length?
But why not leave the key checks the where they are, since it's
dkeys[0..1] that get passed around the implementations?
--D
> -
> info.gfi_next_fsblk = head->fmh_keys[0].fmr_physical +
> head->fmh_keys[0].fmr_length;
> info.gfi_formatter = formatter;
> diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h
> index ac642be2302e..8325258def7b 100644
> --- a/fs/ext4/fsmap.h
> +++ b/fs/ext4/fsmap.h
> @@ -8,6 +8,7 @@
> #define __EXT4_FSMAP_H__
>
> struct fsmap;
> +struct fsmap_head;
>
> /* internal fsmap representation */
> struct ext4_fsmap {
> @@ -32,6 +33,8 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest,
> struct ext4_fsmap *src);
> void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest,
> struct fsmap *src);
> +int ext4_fsmap_check_head(struct super_block *sb,
> + const struct fsmap_head *head);
>
> /* fsmap to userspace formatter - copy to user & advance pointer */
> typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 8067ccda34e4..eb0ecb012e6a 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -874,20 +874,9 @@ static int ext4_ioc_getfsmap(struct super_block *sb,
>
> if (copy_from_user(&head, arg, sizeof(struct fsmap_head)))
> return -EFAULT;
> - if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
> - memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
> - sizeof(head.fmh_keys[0].fmr_reserved)) ||
> - memchr_inv(head.fmh_keys[1].fmr_reserved, 0,
> - sizeof(head.fmh_keys[1].fmr_reserved)))
> - return -EINVAL;
> - /*
> - * ext4 doesn't report file extents at all, so the only valid
> - * file offsets are the magic ones (all zeroes or all ones).
> - */
> - if (head.fmh_keys[0].fmr_offset ||
> - (head.fmh_keys[1].fmr_offset != 0 &&
> - head.fmh_keys[1].fmr_offset != -1ULL))
> - return -EINVAL;
> + error = ext4_fsmap_check_head(sb, &head);
> + if (error)
> + return error;
>
> xhead.fmh_iflags = head.fmh_iflags;
> xhead.fmh_count = head.fmh_count;
> --
> 2.39.2.637.g21b0678d19-goog
>
Hi, Darrick,
Thanks for taking the time to review this patch.
On 3/4/23 02:56, Darrick J. Wong wrote:
> On Wed, Feb 22, 2023 at 01:12:10PM +0000, Tudor Ambarus wrote:
>> Sanity checks should be done the soonest possible to avoid superfluous
>> computations when user provides wrong data. Gather all the checks on
>> user provided data in a single method and call it immediately after
>> copying the data from user.
>
> This patch changes the validation criteria, moves chunks of code around,
The validation criteria remains the same, there's no functional change
in the code.
> and constifies parameters all at once. And all you say here is that
> you're moving validation code up in the sequence!
My apologies, I should have mentioned something about the
constification. I chose to do the validation over const data because
the data should not be changed at validation time, otherwise one may end
with nasty implications on the sequence of validation. The const change
deserved at least a comment if not a dedicated patch, I agree.
>
> Also, how does moving callsites around improve things? Do the fstests
You don't waste CPU cycles in case the validation fails later on in the
code. Every initialization that is done before the last validation check
is superfluous in case the validation fails. Also, having the validation
scattered around copies of user data and in different methods is harder
to follow. What I did was to gather all validation checks in a single
method and call it the soonest possible. IMO this makes the code cleaner
and easier to understand.
> still pass?
Yes, please check the cover letter at:
https://lore.kernel.org/linux-ext4/[email protected]/
All the available ext4 fsmap tests passed after this patch set. I tested
ext4/{027, 028, 029}.
>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++-------------
>> fs/ext4/fsmap.h | 3 +++
>> fs/ext4/ioctl.c | 17 +++-------------
>> 3 files changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
>> index b5289378a761..a27d9f0967b7 100644
>> --- a/fs/ext4/fsmap.c
>> +++ b/fs/ext4/fsmap.c
>> @@ -9,6 +9,7 @@
>> #include "fsmap.h"
>> #include "mballoc.h"
>> #include <linux/sort.h>
>> +#include <linux/string.h>
>> #include <linux/list_sort.h>
>> #include <trace/events/ext4.h>
>>
>> @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
>>
>> /* Do we recognize the device? */
>> static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
>> - struct ext4_fsmap *fm)
>> + const struct fsmap *fm)
>> {
>> if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
>> fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
>> @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
>> }
>>
>> /* Ensure that the low key is less than the high key. */
>> -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
>> - struct ext4_fsmap *high_key)
>> +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key,
>> + const struct fsmap *high_key)
>> {
>> + u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
>> +
>> if (low_key->fmr_device > high_key->fmr_device)
>> return false;
>> if (low_key->fmr_device < high_key->fmr_device)
>> return true;
>>
>> - if (low_key->fmr_physical > high_key->fmr_physical)
>> + if (l_fmr_phys > high_key->fmr_physical)
>> return false;
>> - if (low_key->fmr_physical < high_key->fmr_physical)
>> + if (l_fmr_phys < high_key->fmr_physical)
>
> Why are you changing the comparison here?
So that I preserve the validation check that was done before this patch.
In the code there are 3 representations of the key on which we currently
do validations:
1/ the ones from struct fsmap_head head; -> contains the data copied
from the user
2/ the ones from struct ext4_fsmap_head xhead; -> ext4 internal
representation of the fsmap
3/ dkeys - local keys used to query the device. These are 2/ but with
the low key bumped by fmr_length.
As you correctly identified below, ext4_getfsmap_check_keys() validated
dkeys[0] and fmh_keys[1], so a combination of 2/ and 3/, whereas now I
use it to validate directly the data copied from user, thus the data
from 1/. In order to do that and at the same time to preserve the logic,
I had to introduce a local variable, l_fmr_phys, and bump the low key by
fmr_length.
As you see, now instead of scattering the checks on data from 1/, 2/ and
3/, I do the checks only on the user provided data, thus 1/.
>
>> return true;
>>
>> if (low_key->fmr_owner > high_key->fmr_owner)
>> @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
>> return false;
>> }
>>
>> +int ext4_fsmap_check_head(struct super_block *sb,
>> + const struct fsmap_head *head)
>> +{
>> + const struct fsmap *l = &head->fmh_keys[0];
>> + const struct fsmap *h = &head->fmh_keys[1];
>> +
>> + if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) ||
>> + memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) ||
>> + memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved)))
>> + return -EINVAL;
>> + /*
>> + * ext4 doesn't report file extents at all, so the only valid
>> + * file offsets are the magic ones (all zeroes or all ones).
>> + */
>> + if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL))
>> + return -EINVAL;
>> +
>> + if (head->fmh_iflags & ~FMH_IF_VALID)
>> + return -EINVAL;
>> +
>> + if (!ext4_getfsmap_is_valid_device(sb, l) ||
>> + !ext4_getfsmap_is_valid_device(sb, h))
>> + return -EINVAL;
>> +
>> + if (!ext4_getfsmap_check_keys(l, h))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> #define EXT4_GETFSMAP_DEVS 2
>> /*
>> * Get filesystem's extents as described in head, and format for
>> @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
>> int i;
>> int error = 0;
>>
>> - if (head->fmh_iflags & ~FMH_IF_VALID)
>> - return -EINVAL;
>> - if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
>> - !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
>> - return -EINVAL;
>> -
>> head->fmh_entries = 0;
>>
>> /* Set up our device handlers. */
>> @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
>> dkeys[0].fmr_length = 0;
>> memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap));
>>
>> - if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1]))
>> - return -EINVAL;
>
> And why is it ok to turn validation of dkeys[0] vs. fmh_keys[1] into a
> validation of fmh_keys[0..1] ? I guess that's why check_keys now adds
> the low key physical offset and length?
Yes, you're correct. It's okay to validate directly on the data copied
from user because 2/ and 3/ are just copies of 1/.
>
> But why not leave the key checks the where they are, since it's
> dkeys[0..1] that get passed around the implementations?
>
I hope I made it clear at this point. Waiting for your reply.
Cheers,
ta