daniel/danos
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects 1 Releases Wiki Activity

exfat: adversarial-review fixes — overflow safety, sparse gaps, dir size, big-image bitmap (S4 step 9)

Browse Source 
A 7-dimension adversarial review of the engine, tool, and routing found
nine real defects (host tests + the in-VM drill missed them). Fixed:

- geometryOf now rejects a crafted VBR whose cluster shift exceeds the
  exFAT ceiling (bytes+sectors shift > 25) or whose cluster_count exceeds
  the spec max (0xFFFFFFF5) — either would overflow the engine's u32
  cluster-byte / cluster-bounds arithmetic and panic under ReleaseSafe on
  untrusted removable media. validCluster/allocateCluster widened to u64,
  and writeFile's clusters_needed widened, for a >4 GiB file near the u32
  offset boundary.
- writeFile no longer claims valid_data_length = size unconditionally: a
  sparse write past a foreign file's old valid boundary now zero-fills the
  skipped gap on disk, so a read there returns zero, not stale bytes.
- ensureDirCapacity rewrites a grown subdirectory's own DataLength, so a
  spec-compliant reader that bounds a directory by DataLength sees the new
  entries (danos itself bounds by the end marker, but chkdsk / other OSes
  do not).
- make-exfat-image lays the allocation bitmap across as many clusters as
  it needs; a >128 MiB image (whose bitmap exceeds one cluster) was
  self-inconsistent. Verified: the engine mounts+reads both the 48 MiB
  fixture and a 256 MiB image.

Documented (not fixed here — a shared vfs-layer limit, like the u32
offset cap): non-ASCII names fold to '?', the same as the FAT engine.

New host tests pin each fix (crafted-VBR rejection, sparse-gap zero,
subdir-grows-and-records-size). Full suite 131/131, bounds green.
This commit is contained in:
Daniel Samson
2026-08-10 04:14:54 +01:00
parent 77b64229c2
commit 6ddb08091d
3 changed files with 184 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
system/services/exfat/engine.zig
+121 -16
View File
@@ -183,7 +183,7 @@ pub const FileSystem = struct {
}
fn validCluster(self: *const FileSystem, cluster: u32) bool {
return cluster >= on_disk.first_data_cluster and cluster < self.geometry.cluster_count + on_disk.first_data_cluster;
return cluster >= on_disk.first_data_cluster and cluster < @as(u64, self.geometry.cluster_count) + on_disk.first_data_cluster;
}
/// Read the 32-bit FAT entry for `cluster` (exFAT's FAT is only consulted for a
@@ -393,6 +393,14 @@ pub const FileSystem = struct {
const name_entry = std.mem.bytesToValue(on_disk.FileNameEntry, &fn_raw);
for (name_entry.file_name) |unit| {
if (name_len >= stream.name_length) break;
// KNOWN LIMITATION: danos represents file names as ASCII bytes
// through the whole VFS layer, so a non-ASCII UTF-16 unit becomes
// '?'. This is not an exFAT shortcut — the FAT engine folds LFN
// names the same way, and it is the same class of surface limit
// as the u32 file-offset cap: fixing it means teaching the vfs
// name representation UTF-8, a separate cross-cutting change.
// Consequence on foreign media: distinct non-ASCII names collapse
// to one skeleton and resolve by the true UTF-8 name misses.
if (name_len < name_buf.len) name_buf[name_len] = if (unit < 0x80) @truncate(unit) else '?';
name_len += 1;
}
@@ -527,7 +535,7 @@ pub const FileSystem = struct {
/// Allocate one free cluster (mark its bit), or null if the volume is full.
fn allocateCluster(self: *FileSystem) ?u32 {
var cluster: u32 = on_disk.first_data_cluster;
const end = self.geometry.cluster_count + on_disk.first_data_cluster;
const end: u64 = @as(u64, self.geometry.cluster_count) + on_disk.first_data_cluster;
while (cluster < end) : (cluster += 1) {
if (!self.isAllocated(cluster)) {
return if (self.setAllocated(cluster, true)) cluster else null;
@@ -651,20 +659,33 @@ pub const FileSystem = struct {
const have: u32 = @intCast((@as(u64, dir.size) + cluster_bytes - 1) / cluster_bytes);
return clusters_needed <= have;
}
if (!self.validCluster(dir.first_cluster)) return false;
// Count the existing chain to its last cluster.
var cluster = dir.first_cluster;
if (!self.validCluster(cluster)) return false;
var have: u32 = 1;
while (have < clusters_needed) : (have += 1) {
while (true) {
const next = self.readFatEntry(cluster);
if (self.isEndOfChain(next) or !self.validCluster(next)) {
const fresh = self.allocateCluster() orelse return false;
if (!self.zeroCluster(fresh)) return false;
if (!self.writeFatEntry(cluster, fresh)) return false;
if (!self.writeFatEntry(fresh, on_disk.end_of_chain)) return false;
cluster = fresh;
} else {
cluster = next;
}
if (self.isEndOfChain(next) or !self.validCluster(next)) break;
cluster = next;
have += 1;
}
if (have >= clusters_needed) return true;
// Extend from the last cluster.
while (have < clusters_needed) : (have += 1) {
const fresh = self.allocateCluster() orelse return false;
if (!self.zeroCluster(fresh)) return false;
if (!self.writeFatEntry(cluster, fresh)) return false;
if (!self.writeFatEntry(fresh, on_disk.end_of_chain)) return false;
cluster = fresh;
}
// A grown non-root directory's recorded DataLength must track its chain, or
// a spec-compliant reader that bounds a directory read by DataLength would
// stop before the new entries. The root has no directory entry to update.
if (dir.has_entry) {
var grown = dir;
grown.size = clampU32(@as(u64, have) * cluster_bytes);
grown.valid_data_length = grown.size;
self.updateStream(grown);
}
return true;
}
@@ -778,9 +799,12 @@ pub const FileSystem = struct {
if (data.len == 0) return 0;
const write_len: u32 = @intCast(@min(data.len, @as(usize, std.math.maxInt(u32) - offset)));
if (write_len == 0) return 0;
const old_valid = node.valid_data_length;
if (node.no_fat_chain and !self.ensureFatChain(node)) return 0;
const cluster_bytes = self.clusterBytes();
const clusters_needed = (offset + write_len + cluster_bytes - 1) / cluster_bytes;
// u64 so offset+write_len near 2^32 (a >4 GiB file at the u32 boundary)
// cannot overflow the round-up; the quotient fits u32.
const clusters_needed: u32 = @intCast((@as(u64, offset) + write_len + cluster_bytes - 1) / cluster_bytes);
if (!self.ensureFileClusters(node, clusters_needed)) return 0;
var produced: usize = 0;
@@ -803,12 +827,45 @@ pub const FileSystem = struct {
}
const written_end = offset + @as(u32, @intCast(produced));
if (written_end > node.size) node.size = written_end;
// Every allocated cluster is zeroed, so all bytes up to size are valid.
node.valid_data_length = node.size;
// valid_data_length is the contiguous-from-zero written prefix. A write that
// starts past the old boundary leaves a gap [old_valid, offset) that must
// read as zero — our freshly-allocated clusters are zeroed, but a FOREIGN
// file's existing clusters are not, so zero the gap on disk before claiming
// it valid. (The common append/overwrite path has offset <= old_valid, no
// gap.)
if (offset > old_valid and self.validCluster(node.first_cluster)) {
self.zeroFileRange(node.*, old_valid, offset - old_valid);
}
node.valid_data_length = @max(old_valid, written_end);
self.updateStream(node.*);
return produced;
}
/// Zero `len` bytes of a file starting at `start`, over its existing clusters
/// (the caller has ensured they are allocated). Used to fill a sparse gap.
fn zeroFileRange(self: *FileSystem, node: Node, start: u32, len: u32) void {
const cluster_bytes = self.clusterBytes();
const zero = [_]u8{0} ** sector_size;
var position = start;
var remaining = len;
while (remaining > 0) {
const cluster = self.clusterOfChain(node.first_cluster, node.no_fat_chain, position / cluster_bytes) orelse break;
const in_cluster = position % cluster_bytes;
const lba = self.clusterSector(cluster, in_cluster / sector_size);
const in_sector = in_cluster % sector_size;
const n = @min(remaining, sector_size - in_sector);
if (in_sector == 0 and n == sector_size) {
if (!self.blockWrite(lba, &zero)) break;
} else {
if (!self.blockRead(lba, &self.sector)) break;
@memset(self.sector[in_sector .. in_sector + n], 0);
if (!self.blockWrite(lba, &self.sector)) break;
}
position += n;
remaining -= n;
}
}
/// Empty a file: free its chain and zero its stream.
pub fn truncate(self: *FileSystem, node: *Node) void {
if (self.validCluster(node.first_cluster)) {
@@ -960,6 +1017,7 @@ const test_notes_bytes = 1500; // a three-cluster write in the create/write test
const test_tail_bytes = 4;
const test_inner_bytes = 300;
const test_rename_bytes = 800;
const test_name_bytes = 8; // scratch for the synthetic "F0".."F7" file names
fn testCluster(cluster: u32) usize {
return (test_heap_sector + (cluster - 2)) * sector_size;
@@ -1294,3 +1352,50 @@ test "the exFAT engine rejects a FAT volume (mutual exclusion at mount)" {
formatExfat(bytes);
try std.testing.expect(FileSystem.mount(disk.device()) != null);
}
test "a sparse write leaves the skipped gap reading as zero" {
const allocator = std.testing.allocator;
const bytes = try allocator.alloc(u8, 128 * sector_size);
defer allocator.free(bytes);
var disk = RamDisk{ .bytes = bytes };
formatExfat(bytes);
var fs = FileSystem.mount(disk.device()).?;
fs.current_time_epoch = 1_700_000_000;
var node = fs.createFile(fs.rootNode(), "SPARSE").?;
var tail = [_]u8{0xEE} ** 4;
_ = fs.writeFile(&node, 1000, &tail); // a gap [0, 1000) is never written
const resolved = fs.resolve("/SPARSE").?;
try std.testing.expectEqual(@as(u32, 1004), resolved.size);
var readback: [test_read_bytes]u8 = undefined; // 1024 >= 1004
try std.testing.expectEqual(@as(usize, 1004), fs.readFile(resolved, 0, readback[0..1004]));
for (readback[0..1000]) |b| try std.testing.expectEqual(@as(u8, 0), b); // the gap reads zero
try std.testing.expectEqualSlices(u8, &tail, readback[1000..1004]);
}
test "a subdirectory that outgrows one cluster records its new size" {
const allocator = std.testing.allocator;
const bytes = try allocator.alloc(u8, 128 * sector_size);
defer allocator.free(bytes);
var disk = RamDisk{ .bytes = bytes };
formatExfat(bytes);
var fs = FileSystem.mount(disk.device()).?;
fs.current_time_epoch = 1_700_000_000;
const dir = fs.createDirectory(fs.rootNode(), "BIG").?;
try std.testing.expectEqual(@as(u32, sector_size), dir.size); // one 512-byte cluster
// Each file is a 3-entry set; a 512-byte cluster holds 16 entries, so 8 files
// (24 entries) force the directory onto a second cluster.
var i: u8 = 0;
var namebuf: [test_name_bytes]u8 = undefined;
while (i < 8) : (i += 1) {
const name = std.fmt.bufPrint(&namebuf, "F{d}", .{i}) catch unreachable;
_ = fs.createFile(fs.resolve("/BIG").?, name).?;
}
const grown = fs.resolve("/BIG").?;
try std.testing.expect(grown.size > sector_size); // its recorded DataLength grew with the chain
// Every file is still reachable across the two clusters.
var count: u32 = 0;
while (fs.listEntry(fs.resolve("/BIG").?, count) != null) count += 1;
try std.testing.expectEqual(@as(u32, 8), count);
}
system/services/exfat/on-disk.zig
+30 -2
View File
@@ -201,8 +201,14 @@ pub fn geometryOf(sector: []const u8) ?Geometry {
if (!std.mem.eql(u8, &vbr.filesystem_name, "EXFAT ")) return null;
for (vbr.must_be_zero) |byte| if (byte != 0) return null;
if (vbr.bytes_per_sector_shift < 9 or vbr.bytes_per_sector_shift > 12) return null;
if (vbr.sectors_per_cluster_shift > 25) return null;
if (vbr.number_of_fats == 0 or vbr.cluster_count == 0) return null;
// The exFAT spec caps a cluster at 2^25 bytes (32 MiB): bytes-per-sector-shift
// plus sectors-per-cluster-shift must not exceed 25. Enforcing it here is also
// what keeps the engine's u32 cluster-byte arithmetic (sectors_per_cluster *
// 512) from overflowing on a crafted VBR off untrusted removable media.
if (@as(u16, vbr.bytes_per_sector_shift) + vbr.sectors_per_cluster_shift > 25) return null;
// cluster_count is capped at 0xFFFFFFF5 (the spec's ClusterCount maximum), so
// cluster_count + first_data_cluster cannot overflow u32 in the bounds checks.
if (vbr.number_of_fats == 0 or vbr.cluster_count == 0 or vbr.cluster_count > 0xFFFFFFF5) return null;
if (vbr.first_cluster_of_root < first_data_cluster) return null;
return .{
.bytes_per_sector = @as(u32, 1) << @intCast(vbr.bytes_per_sector_shift),
@@ -396,6 +402,28 @@ test "geometryOf accepts exFAT and the MustBeZero guard rejects a FAT-shaped sec
// Wrong name is rejected too.
sector[3] = 'F';
try std.testing.expect(geometryOf(&sector) == null);
sector[3] = 'E';
}
test "geometryOf rejects crafted VBRs that would overflow u32 cluster arithmetic" {
var sector = [_]u8{0} ** 512;
@memcpy(sector[3..11], "EXFAT ");
sector[510] = 0x55;
sector[511] = 0xAA;
std.mem.writeInt(u32, sector[92..96], 1000, .little); // cluster_count
std.mem.writeInt(u32, sector[96..100], 5, .little); // root cluster
sector[108] = 9; // bytes_per_sector_shift
sector[110] = 1; // number_of_fats
// A cluster shift past the exFAT ceiling (9 + 17 = 26 > 25) would make
// sectors_per_cluster * 512 overflow u32 — rejected.
sector[109] = 17;
try std.testing.expect(geometryOf(&sector) == null);
sector[109] = 3; // sane again
try std.testing.expect(geometryOf(&sector) != null);
// cluster_count above the spec maximum (0xFFFFFFF5) would overflow
// cluster_count + first_data_cluster in the bounds checks — rejected.
std.mem.writeInt(u32, sector[92..96], 0xFFFFFFFF, .little);
try std.testing.expect(geometryOf(&sector) == null);
}
test "set checksum skips its own two bytes and depends on the rest" {