The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH] adfs: reject disc records smaller than one filesystem block
@ 2026-06-05 18:37 Samuel Moelius
  2026-06-10 20:24 ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Samuel Moelius @ 2026-06-05 18:37 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Samuel Moelius, Bae Yeonju, Christian Brauner, Kees Cook, Al Viro,
	open list

ADFS uses the on-disk disc size to report statfs block counts.  The disc
record validator checks the sector size, id length, high disc-size bits,
map zone count, and reserved bytes, but it accepts a declared disc size
smaller than one filesystem block.

A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
checksum validation and mount.  A subsequent statfs then reports zero
f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
while deriving f_ffree.

Reject disc records whose declared disc size is smaller than one
filesystem block.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 fs/adfs/super.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index a4cd0a5159dd..cb8f3919e3bb 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -73,6 +73,10 @@ static int adfs_checkdiscrecord(struct adfs_discrecord *dr)
 	if (le32_to_cpu(dr->disc_size_high) >> dr->log2secsize)
 		return 1;
 
+	/* disc size must contain at least one filesystem block */
+	if (adfs_disc_size(dr) < (1ULL << dr->log2secsize))
+		return 1;
+
 	/*
 	 * Maximum idlen is limited to 16 bits for new directories by
 	 * the three-byte storage of an indirect disc address.  For
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] adfs: reject disc records smaller than one filesystem block
  2026-06-05 18:37 [PATCH] adfs: reject disc records smaller than one filesystem block Samuel Moelius
@ 2026-06-10 20:24 ` Kees Cook
  2026-06-28 22:01   ` [PATCH v2 0/2] " Samuel Moelius
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2026-06-10 20:24 UTC (permalink / raw)
  To: Samuel Moelius
  Cc: Russell King (Oracle), Bae Yeonju, Christian Brauner, Al Viro,
	open list

On Fri, Jun 05, 2026 at 06:37:34PM +0000, Samuel Moelius wrote:
> ADFS uses the on-disk disc size to report statfs block counts.  The disc
> record validator checks the sector size, id length, high disc-size bits,
> map zone count, and reserved bytes, but it accepts a declared disc size
> smaller than one filesystem block.
> 
> A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> checksum validation and mount.  A subsequent statfs then reports zero
> f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> while deriving f_ffree.
> 
> Reject disc records whose declared disc size is smaller than one
> filesystem block.

Can you create a tools/testing/selftests/ script that will generate a
good and bad image and attempt to mount both, validating the filesystem
checking logic you're adding here?

-Kees

> 
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
> ---
>  fs/adfs/super.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/adfs/super.c b/fs/adfs/super.c
> index a4cd0a5159dd..cb8f3919e3bb 100644
> --- a/fs/adfs/super.c
> +++ b/fs/adfs/super.c
> @@ -73,6 +73,10 @@ static int adfs_checkdiscrecord(struct adfs_discrecord *dr)
>  	if (le32_to_cpu(dr->disc_size_high) >> dr->log2secsize)
>  		return 1;
>  
> +	/* disc size must contain at least one filesystem block */
> +	if (adfs_disc_size(dr) < (1ULL << dr->log2secsize))
> +		return 1;
> +
>  	/*
>  	 * Maximum idlen is limited to 16 bits for new directories by
>  	 * the three-byte storage of an indirect disc address.  For
> -- 
> 2.43.0
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 0/2] adfs: reject disc records smaller than one filesystem block
  2026-06-10 20:24 ` Kees Cook
@ 2026-06-28 22:01   ` Samuel Moelius
  2026-06-28 22:01     ` [PATCH v2 1/2] " Samuel Moelius
  2026-06-28 22:01     ` [PATCH v2 2/2] selftests: filesystems: add ADFS mount validation test Samuel Moelius
  0 siblings, 2 replies; 10+ messages in thread
From: Samuel Moelius @ 2026-06-28 22:01 UTC (permalink / raw)
  To: kees; +Cc: brauner, iwasbaeyz, linux-kernel, rmk+kernel, sam.moelius, viro

Hi,

This v2 splits the ADFS fix and the requested selftest into separate
patches.

Patch 1 rejects ADFS disc records whose declared disc size is smaller
than one filesystem block. Such an image can otherwise mount and later
hit a divide-by-zero in statfs because f_blocks is zero.

Patch 2 adds a filesystems/adfs selftest that generates two minimal ADFS
images: a valid image with a one-block disc size, and an otherwise
identical invalid image whose disc size is smaller than one filesystem
block. The test verifies that the valid image mounts and statfs succeeds,
and that the invalid image is rejected at mount time.

Changes since v1:
- Split the selftest into a separate patch.
- Added a tools/testing/selftests/filesystems/adfs test target.
- Added generated-good-image and generated-bad-image mount validation.

Thanks,
Sam

Samuel Moelius (2):
  adfs: reject disc records smaller than one filesystem block
  selftests: filesystems: add ADFS mount validation test

 fs/adfs/super.c                               |   4 +
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/filesystems/adfs/Makefile       |   5 +
 .../filesystems/adfs/adfs_mount_check.sh      | 113 ++++++++++++++++++
 .../testing/selftests/filesystems/adfs/config |   2 +
 5 files changed, 125 insertions(+)
 create mode 100644 tools/testing/selftests/filesystems/adfs/Makefile
 create mode 100755 tools/testing/selftests/filesystems/adfs/adfs_mount_check.sh
 create mode 100644 tools/testing/selftests/filesystems/adfs/config

-- 
2.43.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-28 22:01   ` [PATCH v2 0/2] " Samuel Moelius
@ 2026-06-28 22:01     ` Samuel Moelius
  2026-06-28 23:16       ` Russell King
  2026-06-28 22:01     ` [PATCH v2 2/2] selftests: filesystems: add ADFS mount validation test Samuel Moelius
  1 sibling, 1 reply; 10+ messages in thread
From: Samuel Moelius @ 2026-06-28 22:01 UTC (permalink / raw)
  To: kees; +Cc: brauner, iwasbaeyz, linux-kernel, rmk+kernel, sam.moelius, viro

ADFS uses the on-disk disc size to report statfs block counts.  The disc
record validator checks the sector size, id length, high disc-size bits,
map zone count, and reserved bytes, but it accepts a declared disc size
smaller than one filesystem block.

A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
checksum validation and mount.  A subsequent statfs then reports zero
f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
while deriving f_ffree.

Reject disc records whose declared disc size is smaller than one
filesystem block.

Assisted-by: Codex:gpt-5.5-cyber-preview
---
 fs/adfs/super.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index a4cd0a5159dd..cb8f3919e3bb 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -73,6 +73,10 @@ static int adfs_checkdiscrecord(struct adfs_discrecord *dr)
 	if (le32_to_cpu(dr->disc_size_high) >> dr->log2secsize)
 		return 1;
 
+	/* disc size must contain at least one filesystem block */
+	if (adfs_disc_size(dr) < (1ULL << dr->log2secsize))
+		return 1;
+
 	/*
 	 * Maximum idlen is limited to 16 bits for new directories by
 	 * the three-byte storage of an indirect disc address.  For
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] selftests: filesystems: add ADFS mount validation test
  2026-06-28 22:01   ` [PATCH v2 0/2] " Samuel Moelius
  2026-06-28 22:01     ` [PATCH v2 1/2] " Samuel Moelius
@ 2026-06-28 22:01     ` Samuel Moelius
  1 sibling, 0 replies; 10+ messages in thread
From: Samuel Moelius @ 2026-06-28 22:01 UTC (permalink / raw)
  To: kees; +Cc: brauner, iwasbaeyz, linux-kernel, rmk+kernel, sam.moelius, viro

Add a selftest for ADFS disc record validation.  The test generates a
minimal one-zone ADFS image whose declared disc size is exactly one
filesystem block, mounts it read-only, and verifies that statfs succeeds.

It also generates an otherwise-identical image whose declared disc size
is smaller than one filesystem block, and verifies that the kernel rejects
the mount.  This covers the malformed image rejected by the preceding ADFS
fix without deliberately triggering the old statfs divide-by-zero.

Assisted-by: Codex:gpt-5.5-cyber-preview
---
 tools/testing/selftests/Makefile              |   1 +
 .../selftests/filesystems/adfs/Makefile       |   5 +
 .../filesystems/adfs/adfs_mount_check.sh      | 113 ++++++++++++++++++
 .../testing/selftests/filesystems/adfs/config |   2 +
 4 files changed, 121 insertions(+)
 create mode 100644 tools/testing/selftests/filesystems/adfs/Makefile
 create mode 100755 tools/testing/selftests/filesystems/adfs/adfs_mount_check.sh
 create mode 100644 tools/testing/selftests/filesystems/adfs/config

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 8d4db2241cc2..093cc43a2053 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -31,6 +31,7 @@ TARGETS += efivarfs
 TARGETS += exec
 TARGETS += fchmodat2
 TARGETS += filesystems
+TARGETS += filesystems/adfs
 TARGETS += filesystems/binderfs
 TARGETS += filesystems/epoll
 TARGETS += filesystems/fat
diff --git a/tools/testing/selftests/filesystems/adfs/Makefile b/tools/testing/selftests/filesystems/adfs/Makefile
new file mode 100644
index 000000000000..4b38cd45a945
--- /dev/null
+++ b/tools/testing/selftests/filesystems/adfs/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+TEST_PROGS := adfs_mount_check.sh
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/adfs/adfs_mount_check.sh b/tools/testing/selftests/filesystems/adfs/adfs_mount_check.sh
new file mode 100755
index 000000000000..48fa777bebcc
--- /dev/null
+++ b/tools/testing/selftests/filesystems/adfs/adfs_mount_check.sh
@@ -0,0 +1,113 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Generate minimal ADFS images and verify that mount-time disc record
+# validation accepts an image with one filesystem block and rejects an image
+# whose declared disc size is smaller than one filesystem block.
+
+set -euo pipefail
+
+ksft_skip=4
+tmp_dir="$(mktemp -d /tmp/adfs_mount_check.XXXXXX)"
+mnt_dir="${tmp_dir}/mnt"
+good_img="${tmp_dir}/adfs-good.img"
+bad_img="${tmp_dir}/adfs-bad.img"
+
+cleanup()
+{
+	if command -v mountpoint >/dev/null 2>&1 && mountpoint -q "${mnt_dir}"; then
+		umount "${mnt_dir}" || true
+	fi
+	rm -rf "${tmp_dir}"
+}
+trap cleanup EXIT
+
+skip_all()
+{
+	echo "1..0 # SKIP $*"
+	exit "${ksft_skip}"
+}
+
+fail()
+{
+	echo "not ok $1 - $2"
+	exit 1
+}
+
+require_cmd()
+{
+	command -v "$1" >/dev/null 2>&1 || skip_all "missing $1"
+}
+
+adfs_available()
+{
+	grep -qw adfs /proc/filesystems
+}
+
+make_image()
+{
+	local img="$1"
+	local checksum="$2"
+	local disc_size="$3"
+
+	truncate -s 1M "${img}"
+
+	# One-zone ADFS map header.
+	printf '%b' "${checksum}" | dd of="${img}" bs=1 seek=0 \
+		conv=notrunc status=none
+	printf '%b' '\x00\x00\xff' | dd of="${img}" bs=1 seek=1 \
+		conv=notrunc status=none
+
+	# Disc record fields up to, but not including, disc_size.
+	printf '%b' '\x09\x01\x01\x00\x0c\x09\x00\x00' | dd of="${img}" \
+		bs=1 seek=4 conv=notrunc status=none
+	printf '%b' '\x00\x01\x00\x00\x00\x02\x00\x00' | dd of="${img}" \
+		bs=1 seek=12 conv=notrunc status=none
+
+	printf '%b' "${disc_size}" | dd of="${img}" bs=1 seek=20 \
+		conv=notrunc status=none
+}
+
+if [ "$(id -u)" -ne 0 ]; then
+	skip_all "must be run as root"
+fi
+
+require_cmd dd
+require_cmd grep
+require_cmd mount
+require_cmd mountpoint
+require_cmd stat
+require_cmd truncate
+require_cmd umount
+
+if ! adfs_available; then
+	modprobe adfs 2>/dev/null || true
+fi
+adfs_available || skip_all "adfs filesystem support is unavailable"
+
+mkdir -p "${mnt_dir}"
+
+echo "1..2"
+
+# A one-zone ADFS image whose declared disc size is exactly one 512-byte
+# filesystem block.  The first byte is the ADFS map checksum.
+make_image "${good_img}" '\xe4' '\x00\x02\x00\x00'
+
+if ! mount -t adfs -o ro,loop "${good_img}" "${mnt_dir}" >/dev/null 2>&1; then
+	fail 1 "good image mount failed"
+fi
+if ! stat -f "${mnt_dir}" >/dev/null 2>&1; then
+	fail 1 "good image statfs failed"
+fi
+umount "${mnt_dir}"
+echo "ok 1 - good ADFS image mounted and statfs succeeded"
+
+# Same minimal image, but with disc_size == 1.  This is smaller than the
+# 512-byte filesystem block size encoded by log2secsize and must be rejected.
+make_image "${bad_img}" '\xe5' '\x01\x00\x00\x00'
+
+if mount -t adfs -o ro,loop "${bad_img}" "${mnt_dir}" >/dev/null 2>&1; then
+	umount "${mnt_dir}"
+	fail 2 "bad image mounted"
+fi
+echo "ok 2 - undersized ADFS image rejected"
diff --git a/tools/testing/selftests/filesystems/adfs/config b/tools/testing/selftests/filesystems/adfs/config
new file mode 100644
index 000000000000..7d3cf42f8c99
--- /dev/null
+++ b/tools/testing/selftests/filesystems/adfs/config
@@ -0,0 +1,2 @@
+CONFIG_ADFS_FS=m
+CONFIG_BLK_DEV_LOOP=m
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-28 22:01     ` [PATCH v2 1/2] " Samuel Moelius
@ 2026-06-28 23:16       ` Russell King
  2026-06-29 11:07         ` Samuel Moelius
  0 siblings, 1 reply; 10+ messages in thread
From: Russell King @ 2026-06-28 23:16 UTC (permalink / raw)
  To: Samuel Moelius; +Cc: kees, brauner, iwasbaeyz, linux-kernel, viro

On Sun, Jun 28, 2026 at 10:01:25PM +0000, Samuel Moelius wrote:
> ADFS uses the on-disk disc size to report statfs block counts.  The disc
> record validator checks the sector size, id length, high disc-size bits,
> map zone count, and reserved bytes, but it accepts a declared disc size
> smaller than one filesystem block.
> 
> A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> checksum validation and mount.  A subsequent statfs then reports zero
> f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> while deriving f_ffree.

I think this is still too low.

1. The disc map is nzones sectors long, and can not be in the same
sector as the boot block. The disc record can not share with the map.
This means the minimum is 1 + nzones sectors to fit just the map in.

2. If the disc record was found at 0xc00 rather than 0, then that
will add sectors to the minimum size.

3. The root directory is also necessary, which is 2048 bytes for an
E/F format (format_version=0) or root_size for an F+ format.

So, if we're really trying to avoid mounting something that isn't a
proper image, then just checking that the disc size is at least one
sector isn't sufficient.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-28 23:16       ` Russell King
@ 2026-06-29 11:07         ` Samuel Moelius
  2026-06-29 11:34           ` Russell King
  0 siblings, 1 reply; 10+ messages in thread
From: Samuel Moelius @ 2026-06-29 11:07 UTC (permalink / raw)
  To: Russell King; +Cc: kees, brauner, iwasbaeyz, linux-kernel, viro

On Sun, Jun 28, 2026 at 7:17 PM Russell King <linux@armlinux.org.uk> wrote:
>
> On Sun, Jun 28, 2026 at 10:01:25PM +0000, Samuel Moelius wrote:
> > ADFS uses the on-disk disc size to report statfs block counts.  The disc
> > record validator checks the sector size, id length, high disc-size bits,
> > map zone count, and reserved bytes, but it accepts a declared disc size
> > smaller than one filesystem block.
> >
> > A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> > checksum validation and mount.  A subsequent statfs then reports zero
> > f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> > while deriving f_ffree.
>
> I think this is still too low.
>
> 1. The disc map is nzones sectors long, and can not be in the same
> sector as the boot block. The disc record can not share with the map.
> This means the minimum is 1 + nzones sectors to fit just the map in.
>
> 2. If the disc record was found at 0xc00 rather than 0, then that
> will add sectors to the minimum size.
>
> 3. The root directory is also necessary, which is 2048 bytes for an
> E/F format (format_version=0) or root_size for an F+ format.
>
> So, if we're really trying to avoid mounting something that isn't a
> proper image, then just checking that the disc size is at least one
> sector isn't sufficient.

I understand your points, but the intent of this patch was simply to
eliminate a divide-by-zero. Is it sufficient for that purpose?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-29 11:07         ` Samuel Moelius
@ 2026-06-29 11:34           ` Russell King
  2026-06-29 11:45             ` Samuel Moelius
  0 siblings, 1 reply; 10+ messages in thread
From: Russell King @ 2026-06-29 11:34 UTC (permalink / raw)
  To: Samuel Moelius; +Cc: kees, brauner, iwasbaeyz, linux-kernel, viro

On Mon, Jun 29, 2026 at 07:07:57AM -0400, Samuel Moelius wrote:
> On Sun, Jun 28, 2026 at 7:17 PM Russell King <linux@armlinux.org.uk> wrote:
> >
> > On Sun, Jun 28, 2026 at 10:01:25PM +0000, Samuel Moelius wrote:
> > > ADFS uses the on-disk disc size to report statfs block counts.  The disc
> > > record validator checks the sector size, id length, high disc-size bits,
> > > map zone count, and reserved bytes, but it accepts a declared disc size
> > > smaller than one filesystem block.
> > >
> > > A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> > > checksum validation and mount.  A subsequent statfs then reports zero
> > > f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> > > while deriving f_ffree.
> >
> > I think this is still too low.
> >
> > 1. The disc map is nzones sectors long, and can not be in the same
> > sector as the boot block. The disc record can not share with the map.
> > This means the minimum is 1 + nzones sectors to fit just the map in.
> >
> > 2. If the disc record was found at 0xc00 rather than 0, then that
> > will add sectors to the minimum size.
> >
> > 3. The root directory is also necessary, which is 2048 bytes for an
> > E/F format (format_version=0) or root_size for an F+ format.
> >
> > So, if we're really trying to avoid mounting something that isn't a
> > proper image, then just checking that the disc size is at least one
> > sector isn't sufficient.
> 
> I understand your points, but the intent of this patch was simply to
> eliminate a divide-by-zero. Is it sufficient for that purpose?

While it is, you are also introducing selftests that rely on your
behaviour, where you merge the disc record with the map for both
of your test cases. This should also be rejected, because the bytes
in the disc record will be interpreted as the disc map.

Have you checked what happens if you attempt to access such a bad
image, or are you purely focussed on what an AI bot told you was
a problem, rather than looking at the bigger picture?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-29 11:34           ` Russell King
@ 2026-06-29 11:45             ` Samuel Moelius
  2026-06-29 12:07               ` Russell King
  0 siblings, 1 reply; 10+ messages in thread
From: Samuel Moelius @ 2026-06-29 11:45 UTC (permalink / raw)
  To: Russell King; +Cc: kees, brauner, iwasbaeyz, linux-kernel, viro

On Mon, Jun 29, 2026 at 7:34 AM Russell King <linux@armlinux.org.uk> wrote:
>
> On Mon, Jun 29, 2026 at 07:07:57AM -0400, Samuel Moelius wrote:
> > On Sun, Jun 28, 2026 at 7:17 PM Russell King <linux@armlinux.org.uk> wrote:
> > >
> > > On Sun, Jun 28, 2026 at 10:01:25PM +0000, Samuel Moelius wrote:
> > > > ADFS uses the on-disk disc size to report statfs block counts.  The disc
> > > > record validator checks the sector size, id length, high disc-size bits,
> > > > map zone count, and reserved bytes, but it accepts a declared disc size
> > > > smaller than one filesystem block.
> > > >
> > > > A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> > > > checksum validation and mount.  A subsequent statfs then reports zero
> > > > f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> > > > while deriving f_ffree.
> > >
> > > I think this is still too low.
> > >
> > > 1. The disc map is nzones sectors long, and can not be in the same
> > > sector as the boot block. The disc record can not share with the map.
> > > This means the minimum is 1 + nzones sectors to fit just the map in.
> > >
> > > 2. If the disc record was found at 0xc00 rather than 0, then that
> > > will add sectors to the minimum size.
> > >
> > > 3. The root directory is also necessary, which is 2048 bytes for an
> > > E/F format (format_version=0) or root_size for an F+ format.
> > >
> > > So, if we're really trying to avoid mounting something that isn't a
> > > proper image, then just checking that the disc size is at least one
> > > sector isn't sufficient.
> >
> > I understand your points, but the intent of this patch was simply to
> > eliminate a divide-by-zero. Is it sufficient for that purpose?
>
> While it is, you are also introducing selftests that rely on your
> behaviour, where you merge the disc record with the map for both
> of your test cases. This should also be rejected, because the bytes
> in the disc record will be interpreted as the disc map.
>
> Have you checked what happens if you attempt to access such a bad
> image, or are you purely focussed on what an AI bot told you was
> a problem, rather than looking at the bigger picture?

I have not attempted to access the bad image. And to be clear, the
selftest was an attempt to address Kees Cook's request here:
https://lore.kernel.org/all/202606101323.0DFB06B054@keescook/

> Can you create a tools/testing/selftests/ script that will generate a
> good and bad image and attempt to mount both, validating the filesystem
> checking logic you're adding here?

Would another approach be better?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] adfs: reject disc records smaller than one filesystem block
  2026-06-29 11:45             ` Samuel Moelius
@ 2026-06-29 12:07               ` Russell King
  0 siblings, 0 replies; 10+ messages in thread
From: Russell King @ 2026-06-29 12:07 UTC (permalink / raw)
  To: Samuel Moelius; +Cc: kees, brauner, iwasbaeyz, linux-kernel, viro

On Mon, Jun 29, 2026 at 07:45:21AM -0400, Samuel Moelius wrote:
> On Mon, Jun 29, 2026 at 7:34 AM Russell King <linux@armlinux.org.uk> wrote:
> >
> > On Mon, Jun 29, 2026 at 07:07:57AM -0400, Samuel Moelius wrote:
> > > On Sun, Jun 28, 2026 at 7:17 PM Russell King <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Sun, Jun 28, 2026 at 10:01:25PM +0000, Samuel Moelius wrote:
> > > > > ADFS uses the on-disk disc size to report statfs block counts.  The disc
> > > > > record validator checks the sector size, id length, high disc-size bits,
> > > > > map zone count, and reserved bytes, but it accepts a declared disc size
> > > > > smaller than one filesystem block.
> > > > >
> > > > > A crafted one-zone image with log2secsize 9 and disc_size 1 can pass map
> > > > > checksum validation and mount.  A subsequent statfs then reports zero
> > > > > f_blocks from adfs_map_statfs(), and adfs_statfs() divides by that zero
> > > > > while deriving f_ffree.
> > > >
> > > > I think this is still too low.
> > > >
> > > > 1. The disc map is nzones sectors long, and can not be in the same
> > > > sector as the boot block. The disc record can not share with the map.
> > > > This means the minimum is 1 + nzones sectors to fit just the map in.
> > > >
> > > > 2. If the disc record was found at 0xc00 rather than 0, then that
> > > > will add sectors to the minimum size.
> > > >
> > > > 3. The root directory is also necessary, which is 2048 bytes for an
> > > > E/F format (format_version=0) or root_size for an F+ format.
> > > >
> > > > So, if we're really trying to avoid mounting something that isn't a
> > > > proper image, then just checking that the disc size is at least one
> > > > sector isn't sufficient.
> > >
> > > I understand your points, but the intent of this patch was simply to
> > > eliminate a divide-by-zero. Is it sufficient for that purpose?
> >
> > While it is, you are also introducing selftests that rely on your
> > behaviour, where you merge the disc record with the map for both
> > of your test cases. This should also be rejected, because the bytes
> > in the disc record will be interpreted as the disc map.
> >
> > Have you checked what happens if you attempt to access such a bad
> > image, or are you purely focussed on what an AI bot told you was
> > a problem, rather than looking at the bigger picture?
> 
> I have not attempted to access the bad image. And to be clear, the
> selftest was an attempt to address Kees Cook's request here:
> https://lore.kernel.org/all/202606101323.0DFB06B054@keescook/
> 
> > Can you create a tools/testing/selftests/ script that will generate a
> > good and bad image and attempt to mount both, validating the filesystem
> > checking logic you're adding here?
> 
> Would another approach be better?

So, in essence,

- a bug was found (divide by zero) caused by an invalid image,
  presumably by AI.
- a patch was generated to address _just_ that issue without looking
  at the bigger picture.
- Kees requested a selftest for it
- A selftest was created which _itself_ generates two filesystem
  images, both of which are technically invalid, but tests that one
  mounts but the other doesn't, and the kernel doesn't have the
  divide by zero.
- No check whether the mounted "good" filesystem doesn't cause other
  issues.
- Author resists suggestions from filesystem maintainer to improve
  the check.

Fixing the bigger picture will result in the selftest failing, because
the invalid "good" image is still invalid, and that makes patch 2
incorrect.

Sorry, but no.

While I'm here, another thing that should be checked - that the disc
size specified in the disc record isn't larger than the block device
being mounted.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-06-29 12:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-05 18:37 [PATCH] adfs: reject disc records smaller than one filesystem block Samuel Moelius
2026-06-10 20:24 ` Kees Cook
2026-06-28 22:01   ` [PATCH v2 0/2] " Samuel Moelius
2026-06-28 22:01     ` [PATCH v2 1/2] " Samuel Moelius
2026-06-28 23:16       ` Russell King
2026-06-29 11:07         ` Samuel Moelius
2026-06-29 11:34           ` Russell King
2026-06-29 11:45             ` Samuel Moelius
2026-06-29 12:07               ` Russell King
2026-06-28 22:01     ` [PATCH v2 2/2] selftests: filesystems: add ADFS mount validation test Samuel Moelius

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox