fs: validate ROFS on-disk metadata and fix symlink-path off-by-one

[gburd]

Security fix — kernel heap corruption from a crafted ROFS image + a 1-byte OOB write in symlink resolution. Relevant for any attacker-controlled/attached volume; in a unikernel these are kernel memory corruption.

1. ROFS mount trusts all superblock counts/sizes (fs/rofs/rofs_vfsops.cc)

• alloc_phys_contiguous_aligned(BSIZE * structure_info_blocks_count) — integer overflow + no null check.
• malloc(sizeof(x) * {directory_entries,symlinks,inodes}_count) — overflow to a tiny allocation.
• The directory-entry / symlink / inode parse loops walk data_ptr with no bound against the buffer actually read. A crafted count or per-entry filename_size/symlink_path_size walks far past the buffer → OOB read (heap contents copied into filenames) then a fault.

Fix: checked multiplication, null checks, and an in_bounds() guard on every read from the structure buffer; EINVAL on any inconsistency.

2. ROFS runtime index OOB (fs/rofs/rofs_vnops.cc)

rofs_readlink/readdir/lookup indexed symlinks[], dir_entries[], inodes[] using data_offset / inode_no / d_ino taken straight from the image, unbounded. The readlink guard was an assert() — compiled out in release. inodes + (inode_no - 1) with inode_no==0 underflows.

Fix: validate every index against the parsed table sizes; reject inode_no==0.

3. namei_follow_link off-by-one (fs/vfs/vfs_lookup.cc)

read_link(dp, lp, PATH_MAX, &sz) can set sz==PATH_MAX, then lp[sz]=0 writes 1 byte past the PATH_MAX buffer (symlink target of on-disk size >= PATH_MAX). Fix: read at most PATH_MAX-1.

Severity

High (crafted image → kernel heap corruption) + Low (namei off-by-one). All in shipped master.

Verified: ROFS still mounts and boots to userspace, file access (lookup/readdir/readlink) works normally.

[Copilot]

Pull request overview

This PR hardens ROFS mounting and runtime operations against crafted on-disk metadata to prevent kernel memory corruption, and fixes a 1-byte out-of-bounds write in symlink resolution during path lookup.

Changes:

• Limit read_link() reads to PATH_MAX - 1 to keep the explicit NUL terminator in-bounds.
• Add ROFS superblock-derived size/count validation, overflow checks, allocation checks, and structure-buffer bounds guards during mount.
• Add runtime bounds checks for ROFS table indexing in readlink, readdir, and lookup.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
fs/vfs/vfs_lookup.cc	Fixes namei_follow_link() symlink buffer off-by-one by reading at most PATH_MAX-1.
fs/rofs/rofs_vnops.cc	Adds bounds checks for on-disk-derived indices when accessing ROFS metadata tables at runtime.
fs/rofs/rofs_vfsops.cc	Adds mount-time validation/guardrails for ROFS on-disk metadata parsing and allocations.

Comments suppressed due to low confidence (3)

fs/rofs/rofs_vnops.cc:249

• de_idx is computed as inode->data_offset + idx from on-disk values. This addition can overflow and wrap, which could bypass the de_idx >= directory_entries_count check and lead to an out-of-bounds access in rofs->dir_entries[de_idx].

    for (unsigned int idx = 0; idx < inode->dir_children_count; idx++) {
        uint64_t de_idx = (uint64_t)inode->data_offset + idx;
        if (de_idx >= rofs->sb->directory_entries_count) {
            break;   // directory metadata is inconsistent; stop rather than OOB
        }

fs/rofs/rofs_vfsops.cc:194

• This loop counter is unsigned int while symlinks_count is uint64_t from disk. If symlinks_count > UINT_MAX, idx wraps and the loop may not terminate.

    for (unsigned int idx = 0; idx < sb->symlinks_count; idx++) {
        if (!in_bounds(data_ptr, sizeof(unsigned short))) {

fs/rofs/rofs_vfsops.cc:229

• This loop counter is unsigned int while inodes_count is uint64_t from disk. If inodes_count > UINT_MAX, idx wraps and the loop may not terminate.

    memcpy(rofs->inodes, data_ptr, sb->inodes_count * sizeof(struct rofs_inode));

    for (unsigned int idx = 0; idx < sb->inodes_count; idx++) {
        print("[rofs] inode: %d, size: %d\n", rofs->inodes[idx].inode_no, rofs->inodes[idx].file_size);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gburd]

Thanks Copilot. All four addressed in the current tip (737322e, rebased onto master):

• in_bounds() false positive once past data_end: right, (data_end - q) cast to size_t would wrap to a huge value. The helper now guards the upper bound explicitly (q <= data_end) before computing the difference.
• Loop counters unsigned int vs uint64_t on-disk counts: the directory-entry, symlink, and inode loops now iterate with uint64_t counters matching the on-disk counts, so a crafted directory_entries_count > UINT_MAX cannot wrap the index into an endless loop.
• Mount-failure leaks (sb / rofs / partial allocations): added a single fail() cleanup that frees the partially built arrays (and their inner filename/symlink strings) plus the superblock and the rofs_info on every post-superblock error return; the arrays are now zero-filled (calloc) so the cleanup can safely walk them for inner pointers. Repeated crafted-image mount failures no longer leak kernel memory.
• de_idx = data_offset + index uint64_t wrap: readdir and lookup now detect the wrap (de_idx < inode->data_offset) in addition to the upper-bound check before indexing dir_entries[].

Needs a build to confirm it compiles and the crafted-image path behaves; queued.

[gburd]

Build-validated on x86_64: fs=rofs image=tests builds clean and a ROFS image (760 inodes, 9 symlinks) mounts and reads correctly: tst-readdir-rofs 63/63, tst-symlink-rofs 25/25, tst-chdir-rofs 24/24. The in_bounds guard, uint64_t loop counters, single-cleanup refactor, and symlink off-by-one fix are non-regressing.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

fs/rofs/rofs_vfsops.cc:136

• After copying the superblock into a freshly-allocated sb (heap), the new early-return error paths (bad structure_info_blocks_count, allocation failure, and rofs_read_blocks failure) return without deleting sb. This leaks kernel memory on crafted images that trigger these failures repeatedly.

    uint64_t struct_bytes = (uint64_t)BSIZE * sb->structure_info_blocks_count;
    if (sb->structure_info_blocks_count == 0 ||
        struct_bytes / BSIZE != sb->structure_info_blocks_count ||
        struct_bytes > (SIZE_MAX)) {
        kprintf("[rofs] bad structure_info_blocks_count\n");
        device_close(device);
        return EINVAL;
    }
    buf.reset(alloc_phys_contiguous_aligned(struct_bytes, PAGE_SIZE));
    if (!buf.get()) {
        device_close(device);
        return ENOMEM;
    }
    error = rofs_read_blocks(device, sb->structure_info_first_block, sb->structure_info_blocks_count, buf.get());
    if (error) {
        kprintf("[rofs] Error reading rofs structure info blocks\n");
        device_close(device);
        return error;
    }

[nyh]

I tentatively approve, but I don't understand the security threat here: You can crash the kernel by supplying a crafted filesystem? But if you can change the filesystem, you can also write any executable you want in it. If you can run code that mount()s some bad filesystem, you can also run any code that does anything... What am I missing>