Re: [PATCH 0/1] rust: configfs: Fix reference creation from uninitialized data in `Attribute::show`

[Andreas Hindborg <a.hindborg@kernel.org>]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> Hi Priya,
>
> "Priya Bala Govindasamy" <pgovind2@uci.edu> writes:
>> Dear Linux kernel maintainers,
>>
>> We are developing a tool called FerroLens to detect potential
>> unsound behavior in Rust code in the Linux kernel. The tool
>> internally uses an LLM to detect bugs.
>> We then perform manual analysis to verify these reports.
>> FerroLens reported the following bug in rust/kernel/configfs.rs:
>>
>> When a configfs attribute is written and then read through the
>> same open file, the read reuses the buffer allocated for the write.
>> This buffer is not zero-initialized. Writes to the attribute go
>> through `fill_write_buffer()`, which allocates
>> the buffer with `kmalloc`, and initializes only the input bytes
>> and a trailing NUL byte.
>> The read in `Attribute::show` callback creates an `&mut [u8; PAGE_SIZE]`
>> to the buffer. This is unsound because the reference may include
>> uninitialized bytes.
>>
>> Here is a kernel module and a python script that interacts with it
>> to demonstrate the bug:
>>
>>   // SPDX-License-Identifier: GPL-2.0
>>
>>   //! Rust configfs sample.
>>
>>   use kernel::alloc::flags;
>>   use kernel::configfs;
>>   use kernel::configfs::configfs_attrs;
>>   use kernel::new_mutex;
>>   use kernel::page::PAGE_SIZE;
>>   use kernel::prelude::*;
>>   use kernel::sync::Mutex;
>>
>>   module! {
>>       type: RustConfigfs,
>>       name: "rust_configfs",
>>       authors: ["Rust for Linux Contributors"],
>>       description: "Rust configfs sample",
>>       license: "GPL",
>>   }
>>
>>   #[pin_data]
>>   struct RustConfigfs {
>>       #[pin]
>>       config: configfs::Subsystem<Configuration>,
>>   }
>>
>>   #[pin_data]
>>   struct Configuration {
>>       message: &'static CStr,
>>       #[pin]
>>       bar: Mutex<(KBox<[u8; PAGE_SIZE]>, usize)>,
>>   }
>>
>>   impl Configuration {
>>       fn new() -> impl PinInit<Self, Error> {
>>           try_pin_init!(Self {
>>               message: c"Hello World\n",
>>               bar <- new_mutex!((KBox::new([0; PAGE_SIZE], flags::GFP_KERNEL)?, 0)),
>>           })
>>       }
>>   }
>>
>>   impl kernel::InPlaceModule for RustConfigfs {
>>       fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>>           pr_info!("Rust configfs sample (init)\n");
>>
>>           // Define a subsystem with the data type `Configuration`, two
>>           // attributes, `message` and `bar` and child group type `Child`. `mkdir`
>>           // in the directory representing this subsystem will create directories
>>           // backed by the `Child` type.
>>           let item_type = configfs_attrs! {
>>               container: configfs::Subsystem<Configuration>,
>>               data: Configuration,
>>               child: Child,
>>               attributes: [
>>                   message: 0,
>>                   bar: 1,
>>               ],
>>           };
>>
>>           try_pin_init!(Self {
>>               config <- configfs::Subsystem::new(
>>                   c"rust_configfs", item_type, Configuration::new()
>>               ),
>>           })
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::GroupOperations for Configuration {
>>       type Child = Child;
>>
>>       fn make_group(&self, name: &CStr) -> Result<impl PinInit<configfs::Group<Child>, Error>> {
>>           // Define a group with data type `Child`, one attribute `baz` and child
>>           // group type `GrandChild`. `mkdir` in the directory representing this
>>           // group will create directories backed by the `GrandChild` type.
>>           let tpe = configfs_attrs! {
>>               container: configfs::Group<Child>,
>>               data: Child,
>>               child: GrandChild,
>>               attributes: [
>>                   baz: 0,
>>               ],
>>           };
>>
>>           Ok(configfs::Group::new(name.try_into()?, tpe, Child::new()))
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::AttributeOperations<0> for Configuration {
>>       type Data = Configuration;
>>
>>       fn show(container: &Configuration, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>>           pr_info!("Show message\n");
>>           let data = container.message.to_bytes();
>>           page[0..data.len()].copy_from_slice(data);
>>           Ok(data.len())
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::AttributeOperations<1> for Configuration {
>>       type Data = Configuration;
>>
>>       fn show(container: &Configuration, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>>           pr_info!(
>>               "Show bar: page addr = {:p}, capacity = {} bytes\n",
>>               page.as_ptr(),
>>               page.len()
>>           );
>>
>>           let is_zeroed = page.iter().all(|&b| b == 0);
>>           pr_info!("Show bar: page is zero-initialized: {}\n", is_zeroed);
>>
>>           pr_info!("Show bar: first 8 initial bytes = {:x?}\n", &page[..8]);
>>
>>           let guard = container.bar.lock();
>>           let data = guard.0.as_slice();
>>           let len = guard.1;
>>
>>           if len > PAGE_SIZE {
>>               return Err(kernel::error::code::EINVAL);
>>           }
>>
>>           page[..len].copy_from_slice(&data[..len]);
>>
>>           // 4. Verify post-write state
>>           pr_info!("Show bar: wrote {} bytes into page\n", len);
>>
>>           Ok(len)
>>       }
>>
>>       fn store(container: &Configuration, page: &[u8]) -> Result {
>>           pr_info!(
>>               "Store bar: container={:#x} bar={:#x}\n",
>>               container as *const Configuration as usize,
>>               &container.bar as *const _ as usize,
>>           );
>>           let mut guard = container.bar.lock();
>>           pr_info!(
>>               "Store bar: interpreted buf={:#x} old_len={}\n",
>>               guard.0.as_slice().as_ptr() as usize,
>>               guard.1
>>           );
>>           guard.0[0..page.len()].copy_from_slice(page);
>>           guard.1 = page.len();
>>           Ok(())
>>       }
>>   }
>>
>>   // `pin_data` cannot handle structs without braces.
>>   #[pin_data]
>>   struct Child {}
>>
>>   impl Child {
>>       fn new() -> impl PinInit<Self, Error> {
>>           try_pin_init!(Self {})
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::GroupOperations for Child {
>>       type Child = GrandChild;
>>
>>       fn make_group(&self, name: &CStr) -> Result<impl PinInit<configfs::Group<GrandChild>, Error>> {
>>           // Define a group with data type `GrandChild`, one attribute `gc`. As no
>>           // child type is specified, it will not be possible to create subgroups
>>           // in this group, and `mkdir`in the directory representing this group
>>           // will return an error.
>>           let tpe = configfs_attrs! {
>>               container: configfs::Group<GrandChild>,
>>               data: GrandChild,
>>               attributes: [
>>                   gc: 0,
>>               ],
>>           };
>>
>>           Ok(configfs::Group::new(
>>               name.try_into()?,
>>               tpe,
>>               GrandChild::new(),
>>           ))
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::AttributeOperations<0> for Child {
>>       type Data = Child;
>>
>>       fn show(_container: &Child, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>>           pr_info!("Show baz\n");
>>           let data = c"Hello Baz\n".to_bytes();
>>           page[0..data.len()].copy_from_slice(data);
>>           Ok(data.len())
>>       }
>>   }
>>
>>   // `pin_data` cannot handle structs without braces.
>>   #[pin_data]
>>   struct GrandChild {}
>>
>>   impl GrandChild {
>>       fn new() -> impl PinInit<Self, Error> {
>>           try_pin_init!(Self {})
>>       }
>>   }
>>
>>   #[vtable]
>>   impl configfs::AttributeOperations<0> for GrandChild {
>>       type Data = GrandChild;
>>
>>       fn show(_container: &GrandChild, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>>           pr_info!("Show grand child\n");
>>           let data = c"Hello GC\n".to_bytes();
>>           page[0..data.len()].copy_from_slice(data);
>>           Ok(data.len())
>>       }
>>   }
>>
>> poc.py:
>>
>> ```
>> import os
>>
>> fd = os.open("/sys/kernel/config/rust_configfs/bar", os.O_RDWR)
>> try:
>>     os.write(fd, b"x")
>>     os.lseek(fd, 0, os.SEEK_SET)
>>     print(repr(os.read(fd, 4096)))
>> finally:
>>     os.close(fd)
>> ```
>>
>> output:
>> [  257.079636] rust_configfs: Rust configfs sample (init)
>> [  269.778979] rust_configfs: Store bar: container=0xffffffffa0740100 bar=0xffffffffa0740110
>> [  269.779273] rust_configfs: Store bar: interpreted buf=0xffff888129ce4000 old_len=0
>> [  269.779332] rust_configfs: Show bar: page addr = 0x00000000fca4e938, capacity = 4096 bytes
>> [  269.779352] rust_configfs: Show bar: page is zero-initialized: false
>> [  269.779358] rust_configfs: Show bar: first 8 initial bytes = [78, 0, fb, 4f, 83, 88, ff, ff]
>> [  269.779374] rust_configfs: Show bar: wrote 1 bytes into page
>>
>> This output demonstrates that the bytes of the page that have not
>> been written to by `Attribute::store` are not zero-initialized,
>> which is UB when creating a reference to it.
>
> The buffer is zeroed on allocation during file open, so all the bytes in
> the page are initialized.
>
> The fact that bytes are not zeroed in a write -> seek -> read sequence
> does not mean they are not initialized. They are just not zero.

Actually there may be a problem, but I think it may be in C configfs. If
you take a look at the function that reads data from the iov_iter:

  static int fill_write_buffer(struct configfs_buffer *buffer,
            struct iov_iter *from)
  {
    int copied;

    if (!buffer->page)
      buffer->page = kmalloc(PAGE_SIZE, GFP_KERNEL); // <- HERE
    if (!buffer->page)
      return -ENOMEM;

    copied = copy_from_iter(buffer->page, SIMPLE_ATTR_SIZE - 1, from);
    buffer->needs_read_fill = 1;
    /* if buf is assumed to contain a string, terminate it by \0,
    * so e.g. sscanf() can scan the string easily */
    buffer->page[copied] = 0;
    return copied ? : -EFAULT;
  }

This function does not zero the page that is written into. This,
combined with the buffer being per file handle means that you can read
the original data in the page. The `kmalloc` should be probably be
replaced with a `kzalloc`. This should be a problem for C modules as
well.

Cc: Breno Leitao <leitao@debian.org>

Looks like I only replied Priya when replying, so I'm adding back
everyone from the original email.

Best regards,
Andreas Hindborg