rust: cpufreq: Fix temporary write in Registration::bios_limit_callback

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 issues in rust/kernel/cpufreq.rs.

In Registration::bios_limit_callback, the expression &mut (unsafe { *limit }) creates a reference to a temporary copy of the value pointed to by limit on the stack. Therefore, writes made by T::bios_limit go to this temporary instead of the memory location pointed to by limit.

Additionally, limit may be uninitialized, such as when Registration::bios_limit_callback is invoked by show_bios_limit in drivers/cpufreq/cpufreq.c. Therefore creating a reference from limit is unsound.

Here is a PoC that demonstrates that writes made by T::bios_limit are not persistent:

// SPDX-License-Identifier: GPL-2.0

//! Rust cpufreq proof-of-concept driver for the bios_limit callback bug.

use core::sync::atomic::{AtomicU32, Ordering};

  use kernel::{
      bindings,
      clk::Hertz,
      cpufreq,
      error::code::*,
      macros::vtable,
      prelude::*,
      sync::Arc,
  };

  const LOW_FREQ: Hertz = Hertz::from_mhz(1000);
  const HIGH_FREQ: Hertz = Hertz::from_mhz(2000);
  const BIOS_LIMIT_KHZ: u32 = 424_242;

  module! {
      type: CpuFreqPocModule,
      name: "cpufreq_poc",
      authors: ["Priya Govindasamy"],
      description: "PoC cpufreq driver for the Rust bios_limit callback bug",
      license: "GPL",
  }

  struct CpuFreqPocModule {
      _driver: cpufreq::Registration<CpuFreqPocDriver>,
  }

  struct CpuFreqPocPolicyData {
      table: KVec<bindings::cpufreq_frequency_table>,
      current_freq_khz: AtomicU32,
  }

#[derive(Default)]
struct CpuFreqPocDriver;

  #[vtable]
  impl cpufreq::Driver for CpuFreqPocDriver {
      const NAME: &'static CStr = c"cpufreq-poc";
      const FLAGS: u16 = 0;
      const BOOST_ENABLED: bool = false;

      type PData = Arc<CpuFreqPocPolicyData>;

      fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
          
          let mut table = KVec::new();
          table.push(
              bindings::cpufreq_frequency_table {
                  flags: 0,
                  driver_data: 0,
                  frequency: LOW_FREQ.as_khz() as u32,
              },
              GFP_KERNEL,
          )?;
          table.push(
              bindings::cpufreq_frequency_table {
                  flags: 0,
                  driver_data: 1,
                  frequency: HIGH_FREQ.as_khz() as u32,
              },
              GFP_KERNEL,
          )?;
          table.push(
              bindings::cpufreq_frequency_table {
                  flags: 0,
                  driver_data: 0,
                  frequency: bindings::CPUFREQ_TABLE_END as u32,
              },
              GFP_KERNEL,
          )?;
          
          let cpu = policy.cpu();

          policy.cpus().set(cpu);
          policy
              .set_min(LOW_FREQ)
              .set_max(HIGH_FREQ)
              .set_cpuinfo_min_freq(LOW_FREQ)
              .set_cpuinfo_max_freq(HIGH_FREQ)
              .set_suspend_freq(LOW_FREQ)
              .set_transition_latency_ns(cpufreq::DEFAULT_TRANSITION_LATENCY_NS);

          unsafe { policy.set_freq_table(cpufreq::Table::from_raw(table.as_ptr())) };

          pr_info!(
              "cpufreq_poc: init cpu{} expected bios_limit={} kHz\n",
              cpu.as_u32(),
              BIOS_LIMIT_KHZ
          );

          Ok(Arc::new(
              CpuFreqPocPolicyData {
                  table,
                  current_freq_khz: AtomicU32::new(HIGH_FREQ.as_khz() as u32),
              },
              GFP_KERNEL,
          )?)
      }

      fn verify(data: &mut cpufreq::PolicyData) -> Result {
          data.generic_verify()
      }

      fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result {
          let Some(data) = policy.data::<Self::PData>() else {
              return Err(ENOENT);
          };

          let table = unsafe { cpufreq::Table::from_raw(data.table.as_ptr()) };
          let freq = table.freq(index)?;
          let freq_khz = freq.as_khz() as u32;

          data.current_freq_khz.store(freq_khz, Ordering::Relaxed);
          Ok(())
      }

      fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
          let Some(data) = policy.data::<Self::PData>() else {
              return Err(ENOENT);
          };

          let freq = data.current_freq_khz.load(Ordering::Relaxed);

          Ok(freq)
      }

      fn bios_limit(policy: &mut cpufreq::Policy, limit: &mut u32) -> Result {
          *limit = BIOS_LIMIT_KHZ;

          pr_info!(
              "cpufreq_poc: bios_limit cpu{} wrote {} kHz\n",
              policy.cpu().as_u32(),
              BIOS_LIMIT_KHZ
          );

          Ok(())
      }

      fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result {
          Ok(())
      }

}

  impl kernel::Module for CpuFreqPocModule {
      fn init(_module: &'static ThisModule) -> Result<Self> {
          pr_info!(
              "cpufreq_poc: loading, expected sysfs bios_limit value is {} kHz\n",
              BIOS_LIMIT_KHZ
          );

          let driver = match cpufreq::Registration::<CpuFreqPocDriver>::new() {
              Ok(driver) => driver,
              Err(err) => {
                  pr_err!(
                      "cpufreq_poc: cpufreq driver registration failed: errno={}\n",
                      err.to_errno()
                  );
                  return Err(err);
              }
          };

          Ok(Self { _driver: driver })
      }

}

  impl Drop for CpuFreqPocModule {
      fn drop(&mut self) {
          pr_info!("cpufreq_poc: unloaded\n");
      }
  }

Output:

cat /sys/devices/system/cpu/cpu0/cpufreq/bios_limit 0

[  327.756814] cpufreq_poc: loading out-of-tree module taints kernel.
[  327.756852] cpufreq_poc: module verification failed: signature and/or required key missing - tainting kernel
[  327.761432] cpufreq_poc: cpufreq_poc: loading, expected sysfs bios_limit value is 424242 kHz
[  327.763857] cpufreq_poc: cpufreq_poc: init cpu0 expected bios_limit=424242 kHz
[  327.770173] cpufreq_poc: cpufreq_poc: init cpu1 expected bios_limit=424242 kHz
[  327.777130] cpufreq_poc: cpufreq_poc: init cpu2 expected bios_limit=424242 kHz
[  327.778584] tsc: Marking TSC unstable due to cpufreq changes on SMP
[  327.781279] cpufreq_poc: cpufreq_poc: init cpu3 expected bios_limit=424242 kHz
[  327.782682] cpufreq_poc: cpufreq_poc: init cpu4 expected bios_limit=424242 kHz
[  327.785466] cpufreq_poc: cpufreq_poc: init cpu5 expected bios_limit=424242 kHz
[  327.787647] cpufreq_poc: cpufreq_poc: init cpu6 expected bios_limit=424242 kHz
[  327.789458] cpufreq_poc: cpufreq_poc: init cpu7 expected bios_limit=424242 kHz
[  327.792583] cpufreq_poc: cpufreq_poc: init cpu8 expected bios_limit=424242 kHz
[  327.794484] cpufreq_poc: cpufreq_poc: init cpu9 expected bios_limit=424242 kHz
[  327.797198] cpufreq_poc: cpufreq_poc: init cpu10 expected bios_limit=424242 kHz
[  327.799800] cpufreq_poc: cpufreq_poc: init cpu11 expected bios_limit=424242 kHz
[  327.801306] cpufreq_poc: cpufreq_poc: init cpu12 expected bios_limit=424242 kHz
[  327.804141] cpufreq_poc: cpufreq_poc: init cpu13 expected bios_limit=424242 kHz
[  327.806432] cpufreq_poc: cpufreq_poc: init cpu14 expected bios_limit=424242 kHz
[  327.809162] cpufreq_poc: cpufreq_poc: init cpu15 expected bios_limit=424242 kHz
[  335.494129] cpufreq_poc: cpufreq_poc: bios_limit cpu0 wrote 424242 kHz

Priya Bala Govindasamy (1):
Fix temporary write in Registration::bios_limit_callback

 rust/kernel/cpufreq.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

2.34.1

Writes made by T::bios_limit go to a temporary on the stack instead of the limit pointer passed by C.
Fix by creating a mutable reference to the actual memory location of the limit pointer.

Additionally there is a creation of a reference from the uninitialized limit pointer which is unsound. Fix by initializing the limit pointer to 0 before calling T::bios_limit.

---

 rust/kernel/cpufreq.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 58ac04c650a1..0f9385d41b64 100644
@@ -1349,12 +1349,12 @@ impl<T: Driver> Registration<T> {
     unsafe extern "C" fn bios_limit_callback(cpu: c_int, limit: *mut c_uint) -> c_int {
         // SAFETY: The C API guarantees that `cpu` refers to a valid CPU number.
         let cpu_id = unsafe { CpuId::from_i32_unchecked(cpu) };
-
+        unsafe { core::ptr::write(limit, 0); }
         from_result(|| {
             let mut policy = PolicyCpu::from_cpu(cpu_id)?;
 
             // SAFETY: `limit` is guaranteed by the C code to be valid.
-            T::bios_limit(&mut policy, &mut (unsafe { *limit })).map(|()| 0)
+            T::bios_limit(&mut policy, unsafe { &mut *limit }).map(|()| 0)
         })
     }
 

2.34.1

Why not just use a return value like this?

/// Driver's bios_limit callback.
///
/// Returns the limit.
fn bios_limit(_policy: &mut Policy) -> Result<u32> {

Alice

Makes sense.

Priya, can you make the change as per this please ?

bios_limit_callback() can be adapted as:

unsafe extern "C" fn bios_limit_callback(cpu: c_int, limit: *mut c_uint) -> c_int {
        // SAFETY: The C API guarantees that `cpu` refers to a valid CPU number.
        let cpu_id = unsafe { CpuId::from_i32_unchecked(cpu) };

        from_result(|| {
            let mut policy = PolicyCpu::from_cpu(cpu_id)?;
            let val = T::bios_limit(&mut policy)?;

            // SAFETY: `limit` is guaranteed by the C code to be valid.
            unsafe { core::ptr::write(limit, val); }
            Ok(0)
        })
}

viresh