Rustproofing Linux (Part 3/4 Integer Overflows)

This is a four part blog post series that starts with Rustproofing Linux (Part 1/4 Leaking Addresses).

In the C programming language, integer types can be a bit confusing. Portability issues can arise when the same code is used in multiple hardware architectures or operating systems. For example, int is usually 32-bit, but could also be 16-bit; long is 64-bit on 64-bit architectures, well, except on Windows; and char is normally a signed char, unless you’re on ARM, then it’s an unsigned char.

There are also quite a few integer type promotion rules that define what happens when operations occur on differing types of integers. These nuanced rules can lead to confusion, which is demonstrated by vulnerabilities that were incorrectly fixed and need to be fixed again (CVE-2015-6575 is one such example).

Integer overflows are especially important when checking bounds, and there are many examples of C code where an integer overflow leads to a memory corruption vulnerability.

Again, to demonstrate this bug class, we use a simple driver written in C:

struct entry_data {
    u32 n_entries;
    u8 __user *entries;
};

#define VULN_COPY_ENTRIES _IOW('v', 4, struct entry_data)
#define MAX_ENTRY_SIZE 1024

typedef u8 entries_t[32][MAX_ENTRY_SIZE];

static long vuln_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
    entries_t *entries = filp->private_data;
    struct entry_data entry_data;
    int i;

    switch (cmd) {
    case VULN_COPY_ENTRIES:
        if (copy_from_user( entry_data, (void __user *)arg, sizeof(entry_data)) != 0)
            return -EFAULT;

        if (entry_data.n_entries * MAX_ENTRY_SIZE > sizeof(entries_t)) {
            pr_err("VULN_COPY_ENTRIES: too much entry data (%d)n",
                    entry_data.n_entries * MAX_ENTRY_SIZE);
            return -EINVAL;
        }

        for (i=0; i<entry_data.n_entries; i++) {
            pr_err("idx: %d, ptr: %pxn", i, entries[i]);
            if (copy_from_user((*entries)[i], entry_data.entries+(i*MAX_ENTRY_SIZE), MAX_ENTRY_SIZE) != 0)
                return -EFAULT;
        }
                return 0;
    }

    return -EINVAL;
}

Here, the ioctl command VULN_COPY_ENTRIES is being used to transfer n_entries entries of size MAX_ENTRY_SIZE.

On a quick glance this code might look good, but an observant reader will note that entry_data.n_entries (of type u32 or unsigned int) is first multiplied by MAX_ENTRY_SIZE (of type int) and then checked against sizeof(entries_t). That multiplication could result in a value larger than UINT_MAX which would overflow to a small value and bypass this check, allowing too much data to be copied into the kernel buffer.

We implemented a simple PoC, which also guards against copying the full 4GB+ of data by mapping an unreadable guard page at position where the copying needs to stop. As expected, KASAN catches this write:

root@(none):/# /xxx/rustproofing-linux/poc/test.sh vuln_int_ovf_v2
[  103.623919] ==================================================================
[  103.627426] BUG: KASAN: use-after-free in _copy_from_user+0x35/0x70
[  103.630053] Write of size 1024 at addr ffff888004990000 by task poc_vuln_int_ov/197
[...]
[  103.655332]  kasan_check_range+0x2bd/0x2e0
[  103.657158]  _copy_from_user+0x35/0x70
[  103.658558]  vuln_ioctl+0x149/0x1b0 [vuln_int_ovf_v2]

Apparently there was a freed object immediately after our allocated “entries”, so KASAN misdetected the bug as a use-after-free.

The fix is simple, just remove the problematic multiplication (do not be tempted to cast the operands on the left to size_t, that will not fix the bug if that type is 32-bit):

if (entry_data.n_entries > sizeof(entries_t) / MAX_ENTRY_SIZE) {

Porting to Rust

When we port this code to Rust and execute the PoC, the integer overflow is immediately caught:

root@(none):/# /xxx/rustproofing-linux/poc/test.sh rust_vuln_int_ovf_v2_slice
[  166.026862] rust_kernel: panicked at 'attempt to multiply with overflow', /home/kali/rust/rustproofing-linux/rust_vuln_int_ovf_v2_slice.rs:61:20
[  166.028926] ------------[ cut here ]------------
[  166.029531] kernel BUG at rust/helpers.c:45!

This check is controlled by the config option CONFIG_RUST_OVERFLOW_CHECKS=y, which is enabled by default (in ordinary Rust projects, ‘debug’ builds have “panic on overflow”, while ‘release’ builds have “wrapping” behaviour). Since this option introduces runtime checks that come with a performance hit, it’s not implausible for someone to intentionally disable it. To explore this possibility, we recompiled the kernel with this protection disabled, and tried the PoC again:

root@(none):/# /xxx/rustproofing-linux/poc/test.sh rust_vuln_int_ovf_v2_slice
[   92.767343] rust_vuln_int_ovf_v2_slice: loading out-of-tree module taints kernel.
[   92.806292] rust_kernel: panicked at 'index out of bounds: the len is 32 but the index is 32', /home/kali/rust/rustproofing-linux/rust_vuln_int_ovf_v2_slice.rs:71:50

Good. The overflow was caught by another runtime check. Let’s look a the source code of this Rust version:

fn open(_data:  (), _file:  File) -> Result<Self::Data> {
    Ok(Pin::from(Box::try_new(RustVuln {
        entries: Mutex::new([[0; MAX_ENTRY_SIZE as _]; 32]),
    })?))
}

    VULN_COPY_ENTRIES => {
        let entry_data: EntryData = reader.read()?;

        // CONFIG_RUST_OVERFLOW_CHECKS=y (default) will catch this
        // Note that normally 'debug' builds 'panic on overflow' and 'release' has 'wrapping' behaviour
        if entry_data.n_entries*MAX_ENTRY_SIZE > size_of::<EntriesType>() as u32 {
            pr_err!("VULN_COPY_ENTRIES: too much entry data ({})n", entry_data.n_entries*MAX_ENTRY_SIZE);
            return Err(EINVAL);
        }

        // SAFETY: any source should be safe, since it goes through copy_from_user
        let entry_reader = unsafe { UserSlicePtr::new(entry_data.entries as _, entry_data.n_entries as usize * MAX_ENTRY_SIZE as usize) };
        let mut entry_reader = entry_reader.reader();

        for i in 0..entry_data.n_entries {
            entry_reader.read_slice( mut state.entries.lock()[i as usize])?;
        }
        Ok(0)
    }

The marked line above is the array access that triggered the “index out of bounds” exception, since the allocated array only has 32 rows.

There are quite a few casts (as u32, as usize, and the convenient as _ that can be used where the type can be inferred), which points to the problem not seen in the C code, where integers are automatically promoted.

But can we make it overflow the buffer like we saw in the C code example? The answer is yes! This is possible when raw pointers are used. The starting variant was:

unsafe { entry_reader.read_raw((*state.entries.lock().as_mut_ptr().offset(i as isize)).as_mut_ptr(), MAX_ENTRY_SIZE as usize)? };

It’s not the prettiest line of code. So it can be simplified a bit:

unsafe { entry_reader.read_raw((state.entries.lock().as_mut_ptr().offset(i as isize)) as _, MAX_ENTRY_SIZE as usize)? };

We can make this even cleaner by converting it to a pointer to a slice:

entry_reader.read_slice(unsafe {  mut *state.entries.lock().as_mut_ptr().offset(i as isize) })?;

But the next and quite obvious step has to be the code that directly uses the array. This eliminates the unsafe block which is required by raw pointers, and also demotes the buffer overflow into a runtime array index check:

entry_reader.read_slice( mut state.entries.lock()[i as usize])?;

Takeaways

From the above example it seems hard for an integer overflow to lead to memory corruption. The CONFIG_RUST_OVERFLOW_CHECKS=y option completely prevents the issue, and many casts are a giveaway that the code doing something odd (which admittedly only matters when an auditor is paying attention). In addition, array index checking will catch out-of-bounds array accesses, and a large raw buffer copy is already prevented in the Linux kernel by limiting copy_from_user size to INT_MAX.

A side note. Our very first iteration (Rust version) of this vulnerable driver happened to use global variables. That was soon changed to use an allocation/Box, since accessing global data requires more unsafe keywords, which can be a source of other issues. Weirdly, KASAN did not catch the buffer overflow of a global variable in Rust code, and after a bit of digging we have realised that KASAN is not yet supported (hence all the traces seen here originate from copy_from_user, memset and similar C code).

In the Final Part…

Part 4 about shared memory concludes this blog series.