[asan] SEGV when using the `ReadDir` API

[japaric]

UPDATE: Added Steps To Reproduce (STR) on x86_64

STR1

asan: fs and File clash

use littlefs2::{
    consts, driver,
    fs::{File, Filesystem},
    io::{Result, Write},
    ram_storage,
};

ram_storage!(
    name=RamStorage,
    backend=Ram,
    trait=driver::Storage,
    erase_value=0xff,
    read_size=20*5,
    write_size=20*7,
    cache_size_ty=consts::U700,
    block_size=20*35,
    block_count=32,
    lookaheadwords_size_ty=consts::U1,
    filename_max_plus_one_ty=consts::U256,
    path_max_plus_one_ty=consts::U256,
    result=Result,
);

fn main() {
    let mut ram = Ram::default();
    let mut storage = RamStorage::new(&mut ram);
    let mut alloc = Filesystem::allocate();
    Filesystem::format(&mut storage).unwrap();
    let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap();

    foo(&mut fs, &mut storage);
    bar(&mut fs, &mut storage);
}

#[inline(never)]
fn foo<'r>(fs: &mut Filesystem<RamStorage<'r>>, storage: &mut RamStorage<'r>) {
    fs.read_dir(".", storage).unwrap();
}

#[inline(never)]
fn bar<'r>(fs: &mut Filesystem<RamStorage<'r>>, storage: &mut RamStorage<'r>) {
    let mut fa = File::allocate();
    let mut f = File::create("a.txt", &mut fa, fs, storage).unwrap();
    f.write(fs, storage, b"Hello").unwrap();
    f.close(fs, storage).unwrap();
}

$ # NOTE requires nightly
$ RUSTFLAGS='-Z sanitizer=address' cargo run --target x86_64-unknown-linux-gnu
AddressSanitizer:DEADLYSIGNAL
=================================================================
==16468==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000000b (pc 0x55fc61eeda25 bp 0x7ffe874ef110 sp 0x7ffe874ef110 T0)
==16468==The signal is caused by a READ memory access.
==16468==Hint: address points to the zero page.
    #0 0x55fc61eeda24 in lfs_pair_cmp
    #1 0x55fc61ef0bfd in lfs_dir_commit
    #2 0x55fc61ef2d43 in lfs_file_sync
    #3 0x55fc61ef2564 in lfs_file_close
    #4 0x55fc61ecec80 in littlefs2::fs::File$LT$S$GT$::close
    #5 0x55fc61ed3e14 in hello::bar::hd52d709f8193650b main.rs:45:4
    #6 0x55fc61ed392a in hello::main::ha1cfdc381277c04c main.rs:32:4

---

STR2

msan: fs clashes with itself

use littlefs2::{
    consts, driver,
    fs::{File, Filesystem},
    io::{Result, Write},
    ram_storage,
};

ram_storage!(
    name=RamStorage,
    backend=Ram,
    trait=driver::Storage,
    erase_value=0xff,
    read_size=20*5,
    write_size=20*7,
    cache_size_ty=consts::U700,
    block_size=20*35,
    block_count=32,
    lookaheadwords_size_ty=consts::U1,
    filename_max_plus_one_ty=consts::U256,
    path_max_plus_one_ty=consts::U256,
    result=Result,
);

fn main() {
    let mut ram = Ram::default();
    let mut storage = RamStorage::new(&mut ram);
    let mut alloc = Filesystem::allocate();
    Filesystem::format(&mut storage).unwrap();
    let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap();

    let mut fa = File::allocate();
    let mut f = File::create("a.txt", &mut fa, &mut fs, &mut storage).unwrap();
    f.write(&mut fs, &mut storage, b"Hello").unwrap();
    f.close(&mut fs, &mut storage).unwrap();
    fs.read_dir(".", &mut storage).unwrap(); // causes the next statement to SEGV
    fs.remove("a.txt", &mut storage).unwrap();
}

$ RUSTFLAGS='-Z sanitizer=memory' cargo run --target x86_64-unknown-linux-gnu
MemorySanitizer:DEADLYSIGNAL
==22276==ERROR: MemorySanitizer: SEGV on unknown address 0x000000000011 (pc 0x55ec9c483e8b bp 0x7ffed398b4d0 sp 0x7ffed398b4d0 T22276)
==22276==The signal is caused by a READ memory access.
==22276==Hint: address points to the zero page.
    #0 0x55ec9c483e8a in lfs_pair_cmp
    #1 0x55ec9c487063 in lfs_dir_commit
    #2 0x55ec9c489b2c in lfs_remove
    #3 0x55ec9c4689f7 in littlefs2::fs::Filesystem$LT$Storage$GT$::remove
    #4 0x55ec9c46b81c in hello::main::ha1cfdc381277c04c src/main.rs:36:4

---

Original report

Given this directory structure:

.
├── a.txt
├── b.txt
└── c.txt

I'm observing the following pseudo-code hang:

// works fine
for entry in fs.read_dir(".")? {
    let entry = entry?;
    println!("{:?}", entry.file_type());
}

// NOTE omitting FileAlloc for simplicity
let mut f1 = File::open(&mut fs, "a.txt")?;
f1.write("some text")?;

let mut f2 = File::open(&mut fs, "b.txt")?;
f2.write("more text")?;

f1.close()?; // program hangs here
f2.close()?; // this statement is never reached

And a slightly different program causes the device to hard fault:

File::create(&mut fs, "a.txt")?.close()?;
File::create(&mut fs, "b.txt")?.close()?;
File::create(&mut fs, "c.txt")?.close()?;

for entry in fs.read_dir(".")? {
    let entry = entry?;
    println!("{:?}", entry.file_type());
}

let mut f1 = File::open(&mut fs, "a.txt")?;
f1.write("some text")?;

let mut f2 = File::open(&mut fs, "b.txt")?;
f2.write("more text")?;

// program hard faults while executing one of these statements
f1.close()?;
f2.close()?;
f3.close()?;

---

If I'm reading https://github.com/ARMmbed/littlefs/issues/164#issuecomment-481553076 correctly not closing either a file or a directory can result in UB.

In the case of ReadDir, the lfs_dir_create C function is called but its counterpart, lfs_dir_close, is never called.

Because mem::forget (and other ways of leaking memory) are considered "safe" (don't require unsafe to call), calling lfs_dir_close from ReadDir's destructor is not going to be sufficient to fix this soundness hole. See below:

let read_dir = fs.read_dir(".")?; // safe API
mem::forget(read_dir); // safe API
// code that follows is UB

I think the API would need to become closure-based to ensure lfs_dir_close is always called. Something like this:

impl Filesystem {
    pub fn read_dir(&mut self, on_each_entry: impl FnMut(DirEntry)) -> io::Result<()> {
        // call lfs_dir_open
        // iterate over the contents of the directory; pass each entry to the closure
        // call lfs_dir_close
    }
}

If the same issue (forget to close == UB) applies to Files then a closure-based API would also be needed there to avoid UB in safe code.

As an additional note: is one allowed to modify the directory structure (e.g. call Filesystem::remove) while iterating it with read_dir? It's possible to overlap these operations with the current ReadDir API but the overlap of those actions could be rejected at compile time if ReadDir mutably borrowed Filesystem. I couldn't find an answer to this question myself -- where is littlefs API documented? One of the header files contains what appear to be doc comments but those don't describe what one is allowed to or not to do with the API.

[japaric]

I tried to fix the UB in the issue description example by having ReadDir's destructor call lfs_dir_close but that didn't solve the issue. My code is a bit more complex though because I have a arena of files.

I have done some more digging and have confirmed that not closing files can also cause UB. The following snippet which only uses the File API hangs:

fn main() {
    let mut fs = Filesystem::mount(&mut fsa, &mut storage).unwrap();

    foo(&mut fs, &mut storage);
    bar(&mut fs, &mut storage);
}

#[inline(never)]
fn foo<S>(fs: &mut Filesystem<S>, storage: &mut S) {
    let mut fa = File::allocate();
    forget(File::create("a.txt", &mut fa, fs, storage).unwrap());
}

#[inline(never)]
fn bar<S>(fs: &mut Filesystem<S>, storage: &mut S) {
    let mut fa = File::allocate();
    let mut f = File::create("b.txt", &mut fa, fs, storage).unwrap();
    f.write(fs, storage, b"Hello").unwrap();
    f.close(fs, storage).unwrap(); // program hangs here
}

AFAICT, when you open a file littlefs uses the FileAllocation as a linked list node and appends the open file to a linked list of files (and directories?) it keeps track of. When you close the file littlefs removes the file from its linked list. If you don't close the file and free or move the FileAllocation that causes the linked list node, and the whole list, to become corrupted which leads to UB.

[japaric]

I have updated the issue description to include a repro test case that you can run on x86_64 Linux and triggers a SIGSEV when run under asan (LLVM's AddressSanitizer).

I have opened a separate ticket (#5) for the File.close soundness hole; that one also contains a repro test case that triggers a SIGSEV under asan.

[japaric]

Update: I have added another repro case that's not caught be asan (it exits with return code 0 = sucess), but segfaults when run without instrumentation. The UB is, however, caught by msan.

I tried to fix the UB in the issue description example by having ReadDir's destructor call lfs_dir_close but that didn't solve the issue.

And I think I now see why that was not enough to fix the UB. It seems that the UB is in the Fs.read_dir method implementation:

    pub fn read_dir<P: Into<Path<Storage>>>(
        &mut self,
        path: P,
        storage: &mut Storage,
    ) ->
        Result<ReadDir<Storage>>
    {
        self.alloc.config.context = storage as *mut _ as *mut cty::c_void;

        let mut read_dir = ReadDir {
            state: unsafe { mem::MaybeUninit::zeroed().assume_init() },
            _storage: PhantomData,
        };

        let return_code = unsafe {
            ll::lfs_dir_open(
                &mut self.alloc.state,
                &mut read_dir.state,
                &path.into() as *const _ as *const cty::c_char,
            )
        };

        Error::result_from(return_code).map(|_| read_dir)
    }

lfs_dir_open seems to turn read_dir.state into a linked list node that gets appended into a linked listed stored in self.alloc.state. However, read_dir is a stack allocated value so when the read_dir method returns the stack location of read_dir changes; this causes the linked list in self.alloc.state to point into deallocated memory, which is UB.

Fixing this one seems like it's going to require adding a ReadDirAlloc type / argument to the method so that the ReadDir.state value is not moved when calling read_dir (but that approach is likely still going to run into problems with lfs_dir_close and destructors not being guaranteed to run).

[nickray]

I've started to fix this: https://github.com/nickray/littlefs2/pull/6

I think the UB issues can be summed up with: Use closures to do cleanup (closing files/readdirs) with error reporting.

Will test against my use cases. One thing I'd like but can't currently is directly read the files that are being iterated over in a ReadDir.

[nickray]

@japaric I believe these issues are all prevented/solved in https://github.com/nickray/littlefs2/pull/6.

As an additional note: is one allowed to modify the directory structure (e.g. call Filesystem::remove) while iterating it with read_dir? It's possible to overlap these operations with the current ReadDir API but the overlap of those actions could be rejected at compile time if ReadDir mutably borrowed Filesystem. I couldn't find an answer to this question myself -- where is littlefs API documented? One of the header files contains what appear to be doc comments but those don't describe what one is allowed to or not to do with the API.

I don't know of any documentation on what's allowed :/

I operate under the same mental model of a linked list of open file or directory objects. Personally, I at minimum need to read files during iteration, to filter based on contents. It would also be nice to delete files in a directory based on some condition on the file...

Note that littlefs has more operations on directories than Rust-style read_dir, e.g. you can lfs_dir_{tell,seek,rewind}.

My plan if anything turns up that must be prevented is to have read_dir_and_then pass on a RestrictedFilesystem, that contains a &mut to the actual Filesystem, and only exposes (by delegation) the methods that are safe to use.

I can imagine that removeing the "next" file could lead to an issue (as one can remove the "current" file, so it probably navigates via the "next" pointer in the lfs_file_t, https://github.com/ARMmbed/littlefs/blob/master/lfs.h#L329-L330)

[nickray]

I opened to https://github.com/nickray/littlefs2/issues/7 re. ReadDir. I think/hope this issue can be closed in favor of the former, pending merge of https://github.com/nickray/littlefs2/pull/6.