Skip to content

Out-of-bounds read in KeyRing::get_links when the buffer is too small #22

Open

Description

@vilie

TL;DR; get_links gives the kernel the buffer size in entries instead of bytes and then trusts the length the kernel returns, so a too-small max makes it read past its buffer. That returns wrong links and can crash the process (undefined behavior in safe code).

Repro:
 linux-keyutils 0.2.5, Linux 6.6, debug build (release builds skip the check, so the out-of-bounds read goes unnoticed).

// Cargo.toml: linux-keyutils = "=0.2.5", libc = "0.2"
use linux_keyutils::{KeyRing, KeyRingIdentifier};

fn main() {
    unsafe { libc::syscall(libc::SYS_keyctl, libc::KEYCTL_JOIN_SESSION_KEYRING, 0) };
    let ring = KeyRing::from_special_id(KeyRingIdentifier::Session, false).unwrap();
    for i in 0..50 {
        ring.add_key(&format!("key{i}"), b"x").unwrap();
    }
    println!("{:?}", ring.get_links(4).map(|l| l.len()));
}

Stacktrace (some parts redacted for privacy)

thread 'main' (1871) panicked at <REDACTED>/library/alloc/src/vec/mod.rs:1941:9:
unsafe precondition(s) violated: Vec::set_len requires that new_len <= capacity()

This indicates a bug in the program. This Undefined Behavior check is optional, and cannot be relied on for safety.
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_nounwind_fmt::runtime
   2: core::panicking::panic_nounwind_fmt
   3: <alloc::vec::Vec<_, _>>::set_len::precondition_check
             at <REDACTED>/library/core/src/ub_checks.rs:73:21
   4: <alloc::vec::Vec<linux_keyutils::ffi::types::KeySerialId>>::set_len
             at <REDACTED>/library/alloc/src/vec/mod.rs:1941:9
   5: <linux_keyutils::keyring::KeyRing>::get_links
             at <REDACTED>/linux-keyutils-0.2.5/src/keyring.rs:177:20
   6: rust_report::main
             at ./src/main.rs:11:52
   7: <fn() as core::ops::function::FnOnce<()>>::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
Aborted (core dumped)

Fix: pass the buffer size to the kernel in bytes, and return an error instead of calling set_len()  when the kernel reports the buffer was too small. Is this approach fine? Can send a PR if everything is fine.

Activity

  1. 197g commented on Nov 3, 2026

    @197g

    #23 instead returns the partial results which I'd want to motivate via comment. This is at least correct: The kernel actually does partially fill it and the interface does not return the underlying key ring size in any case. There is a stability concern as it'd present a rather spurious error path that will only appear on a loaded system but not in smoke tests would be stacking risks. I would argue we should roughly follow the kernel here and avoid the hard error, although the risk of silent truncation can't be readily diagnosed.

    The strongest counter argument might be that it this hides a potential error but there are other methods and alternative signatures that could be introduced which are better suited for fixing the underlying ergonomics problem; and those would then also mitigate that gap. Adding them later while preserving the brittle error behavior would be a long standing problem, patching the unsoundness and then adding them would only have people that likely need to patch their code patch their code differently.The other choices would also improve buffer reuse and separate concerns (i.e. reading the ID, initializing the argument buffers, and retrieving metadata).

    Shortly, I'd appreciate something like this:

    impl KeyRing {
        pub fn len(&self) -> Result<usize, KeyError> {
            // … pass an empty buffer
        }
    
        /// Always the number of items in the ring on success.
        pub fn read_items(&self, buf: impl AsMut<[KeySerialId]>) -> Result<usize, KeyError> {
            // …
        }
    
        /// Always the number of items in the ring on success.
        ///
        /// Specialized interface to avoid the initial initialization.. Really optional.
        pub fn read_items_within_capacity(&self, buf: &mut Vec<KeySerialId>) -> Result<usize, KeyError> {
            // …
        }
    }
    
    impl Links {
        /// Internally always allocates. Use [`KeyRing::read_items`] with
        /// [`Self::extend_from_slice`] for more control over that.
        ///
        /// Design note: does not return the keyring item count, assuming you're only
        /// interested Links anyways.
        pub fn extend(&mut self, buf: &KeyRing, max: usize) -> Result<(), KeyError> {
            // …
        }
    
        pub fn extend_from_slice(&mut self, buf: &[KeySerialId]) -> Result<(), KeyError> {
            // …
        }
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions