rust-osdev / volatile Goto Github PK

The ReadWrite type is functionally equivalent to the Volatile type. Is there any reason for having this, or is it for consistency's sake with ReadOnly and WriteOnly (in case the semantics of Volatile change)?

From the doc of Volatile::new

While it is possible to construct Volatile instances from arbitrary values (including non-reference values), most of the methods are only available when the wrapped type is a reference. The only reason that we don't forbid non-reference types in the constructor functions is that the Rust compiler does not support trait bounds on generic const functions yet. When this becomes possible, we will release a new version of this library with removed support for non-references. For these reasons it is not recommended to use the Volatile type only with references.

Should the last sentence be "For these reasons it is recommended to use the Volatile type only with references."? (remove not)

VolatilePtr is used for memory-mapped I/O where lifetimes are not useful. Also, as a volatile counterpart to *-pointers, it would make sense if it didn't have a lifetime either.

It seems it was added in c3d6b9a to make map_mut sound, but that function has been removed again and today's map function can easily be used to map from a 'static to another 'static VolatilePtr.
Does it still serve a purpose?

I'm from Google working with the OpenSSF to improve supply-chain security in many open source projects.

I would like to suggest to volatile project to set minimal scoped permissions to its github workflows (in this case the build.yml file). This means setting the permissions as read only on the top level and any write permission be given at the run level.

This is necessary due to a behavior of github workflow to grant to GITHUB_TOKEN write permissions to all types of permissions, regardless of they being used or not. In case of the workflow getting compromised, an attacker can exploit this permissions.

This can be seen in the Action run step "Set up job" such as https://github.com/rust-osdev/volatile/actions/runs/3974296184.

Considering the changes are quite simple, I'll also suggest the PR together with this issue.
Thanks!

@phil-opp, let me check this before creating a release.

Some people expressed interest in a volatile Cell-like type that owns the value, similar to the v0.3 interface of this crate. I think that such a type would be a nice addition to the pointer types proposed in #29, but the question is whether it is possible to implement such a type in a sound way.

The fundamental issue is that references are marked as dereferenceable by the compiler, which permits LLVM to insert spurious reads. This is not valid for volatile values since they might change in between reads. This issue was e.g. discussed in this thread on the Rust internals forum in 2019, with the conclusion that such a type would require special language support.

About half a year ago, rust-lang/rust#98017 was merged, which (if I understand it correctly) removed the dereferenceable annotation from &T where T contains an UnsafeCell. Maybe this change makes it possible to safely implement a VolatileCell with an inner UnsafeCell now?

As pointed out in japaric/vcell#10, crates like this that use references to mmio are unsound.

I propose a new design that only stores raw pointers to prevent this behavior.
We could then initialize the cell using addr_of_mut to avoid any creation of references before that data is initialized.

pub struct VolatileCell<T> {
    ptr: *mut T,
}

impl<T> VolatileCell<T> {
    pub fn new(ptr: *mut T) -> Self {
        Self { ptr }
    }

    pub fn get(&self) -> T
    where
        T: Copy,
    {
        unsafe { self.ptr.read_volatile() }
    }

    pub fn get_mut(&self) -> T
    where
        T: Copy,
    {
        unsafe { self.ptr.read_volatile() }
    }
}

I was browsing the documentation and noticed that map takes F: FnOnce(&'a T) -> &'a U and T: 'a, but map_mut takes F: FnOnce(&mut T) -> &mut U and T: 'a and wondered about the difference?

By using the unstable features of this crate, it's possible to cause a segmentation fault without writing any unsafe code. I wrote an example below. The reason is that the runtime checks of copy_into_slice and similar methods optimistically assume that the implementation of Deref and DerefMut always returns the same reference. To fix this, I think that there should be at most one call to self.reference.{deref|deref_mut}() in each of those methods.

src/main.rs:

use std::cell::Cell;
use std::ops::Deref;
use volatile::Volatile;

#[repr(C)]
struct Pair {
    counter: Cell<u32>,
    long: [u8; 1000],
    short: [u8; 1],
}

impl Pair {
    fn new() -> Pair {
        Pair {
            counter: Cell::new(0),
            long: [0; 1000],
            short: [0; 1],
        }
    }
}

impl Deref for Pair {
    type Target = [u8];

    fn deref(&self) -> &[u8] {
        let c = self.counter.get();
        self.counter.set(c + 1);

        if c % 3 == 0 {
            &self.short
        } else {
            &self.long
        }
    }
}

fn main() {
    let volatile = Volatile::new(Pair::new());
    let mut dst: [u8; 1] = [1];
    // This will try to write `long` into `dst`, going out of bounds
    volatile.copy_into_slice(&mut dst);
}

Cargo.toml:

[package]
name = "volatile_bug"
version = "0.1.0"
edition = "2021"

[dependencies]
volatile = { version = "0.4.5", features = ["unstable"] }

Output of cargo run:

Segmentation fault (core dumped)

japaric's volatile-register, which is using japaric's vcell

The const_generics feature has recently been removed in favor of adt_const_params, which breaks unstable builds the most recent nightly. I believe the fix is as simple as replacing #![cfg_attr(feature = "unstable", feature(const_generics))] with #![cfg_attr(feature = "unstable", feature(adt_const_params))]

I think there's a way to combine the APIs of 0.3 and 0.4, to be able to have both easy volatile references as well as a substitute for C's volatile typ var; variable declarations (which I was primarily using volatile 0.3 for). I'm imagining something similar to this:

pub struct VolatileRef<R, A> {
    reference: R,
    access: PhantomData<A>,
}
// same as current Volatile functions, except for below name changes
impl<R> VolatileRef<R, ReadWrite> {
    // or maybe from_ref?
    pub const fn new_ref(reference: R) -> Self;
}
impl<R> VolatileRef<R, ReadOnly> {
    pub const fn new_ref_read_only(reference: R) -> Self;
}
impl<R> VolatileRef<R, WriteOnly> {
    pub const fn new_ref_write_only(reference: R) -> Self;
}

// not sure about the name. S for SimpleBox/StackBox, or maybe could do VBox?
#[repr(transparent)]
pub struct SBox<T: ?Sized>(T);

pub type Volatile<T, A> = VolatileRef<SBox<T>, A>;

impl<T> Volatile<T> {
    pub const fn new(value: T) -> Self;
}
impl<T> Volatile<T, ReadOnly> {
    pub const fn new_read_only(value: T) -> Self;
}
impl<T> Volatile<T, WriteOnly> {
    pub const fn new_write_only(value: T) -> Self;
}

I'm totally not sold on any of the naming, but I think the basic idea of Volatile<T, A>/VolatileRef<R, A> is pretty good.

So I have been following the 'Writing an OS in Rust' guide. And while I was experimenting and reading through the code I noticed that Copy hasn't been implemented for Volatile.

This could be useful when writing code:

impl<T: Copy> Copy for Volatile<T> {
    // Empty.
}

After this the new line function for the VGA buffer can be implemented as follows:

fn new_line(&mut self) {
    &self.buffer.chars.copy_within(1.., 0);
        
    self.clear_row(BUFFER_HEIGHT - 1);
    self.column_position = 0;
}

Now I did notice that copy_within uses the normal ptr::copy instead of volatile_copy_memory. Is this a potential issue?

Anyway let me know if you want to add this, if so I will make a short MR.

Hey there,

would it be possible, to provide a short but complete usage example, that allows, for example reading a volatile array?

From reading the source and the documentation I don't really get, how this thing works and how to make it perform volatile reads.

Proposed by @mkroening in #29 (comment).

WriteOnly simply derives Clone so it calls Volatile::clone to clone its field,
https://github.com/embed-rs/volatile/blob/a5a6d786995df7cf07ed8689f79805da10ba2429/src/lib.rs#L210-L211
but Volatile::clone uses Volatile::read internally which might not work since the value is write-only.
https://github.com/embed-rs/volatile/blob/a5a6d786995df7cf07ed8689f79805da10ba2429/src/lib.rs#L144-L148

Removing Clone from WriteOnly is obviously a breaking change, so I'm not sure what to do about this.

A Security Policy is a GitHub standard document (SECURITY.md) that can be seen in the "Security Tab" to instruct users about how to report vulnerability in the safest and most efficient way possible.

It is a Scorecard Recommendation (being one check of medium priority) and a Github Recommendation.

Together with this issue I'll submit one suggestion of Security Policy, feel free to edit it directly or ask me for editions until it is in compliance with how volatile would best handle vulnerability reports.

This crate could be generalized by abstracting VolatileRef and VolatilePtr over the set of memory access operations to be used. The core idea of declaring a region of memory to be safe to access with a certain set of memory access operations; assigning it type; and safely refining it using field access and slice indexing operations is useful beyond just those cases where volatile operations are appropriate.

Here is a sketch of the idea:

#[repr(transparent)]
pub struct AbstractRef<'a, T, O, A = ReadWrite>
where
    T: ?Sized,
{
    pointer: NonNull<T>,
    reference: PhantomData<&'a T>,
    ops: PhantomData<O>,
    access: PhantomData<A>,
}

#[repr(transparent)]
pub struct AbstractPtr<'a, T, O, A = ReadWrite>
where
    T: ?Sized,
{
    pointer: NonNull<T>,
    reference: PhantomData<&'a T>,
    ops: PhantomData<O>,
    access: PhantomData<A>,
}

pub trait Ops: Copy + Default {}

pub trait UnitaryOps<T>: Ops {
    unsafe fn read(src: *const T) -> T;
    unsafe fn write(dst: *mut T, src: T);
}

pub trait BulkOps<T>: Ops {
    unsafe fn memmove(dst: *mut T, src: *const T, count: usize);
    unsafe fn memcpy(dst: *mut T, src: *const T, count: usize);
    unsafe fn memset(dst: *mut T, val: u8, count: usize);
}

#[derive(Default, Copy, Clone)]
pub struct VolatileOps;

impl Ops for VolatileOps {}

impl<T> UnitaryOps<T> for VolatileOps {
    unsafe fn read(src: *const T) -> T {
        unsafe { ptr::read_volatile(src) }
    }

    unsafe fn write(dst: *mut T, src: T) {
        unsafe { ptr::write_volatile(dst, src) }
    }
}

#[cfg(feature = "unstable")]
impl<T> BulkOps<T> for VolatileOps {
    unsafe fn memmove(dst: *mut T, src: *const T, count: usize) {
        unsafe { intrinsics::volatile_copy_memory(dst, src, count) }
    }

    unsafe fn memcpy(dst: *mut T, src: *const T, count: usize) {
        unsafe { intrinsics::volatile_copy_nonoverlapping_memory(dst, src, count) }
    }

    unsafe fn memset(dst: *mut T, val: u8, count: usize) {
        unsafe { intrinsics::volatile_set_memory(dst, val, count) }
    }
}

pub type VolatileRef<'a, T, A = ReadWrite> = AbstractRef<'a, T, VolatileOps, A>;
pub type VolatilePtr<'a, T, A = ReadWrite> = AbstractPtr<'a, T, VolatileOps, A>;

I work on Rust support for seL4 userspace [1]. I’ve been using a branch [2] of the volatile crate, with this proposal implemented, for regions of memory shared with untrusted neighbor processes, and for DMA. In most of these cases, the type of the top-level AbstractRef is either some kind of buffer queue or a large slice of bytes used as a pool for buffers.

It turns out that the story for accessing memory outside of Rust's memory model (e.g. MMIO or externally shared memory such as in the case of DMA) is quite complicated, and many aspects are still up in the air [3][4]. A design like the one I'm proposing allows library users to choose for themselves which memory operations to use, taking into account not only the basic character of the memory in question, but also their particular hardware and their tolerance for undefined behavior.

For the case of externally shared memory, for example, Fuchsia opts to use normal pointer operations with some extra tricks to prevent unwanted optimization [5]. The thread in [3], however, suggests that using normal pointer operations in a context like this results in undefined behavior, and that using LLVM's unordered atomic intrinsics is safe (or at least safer). However, also according to that thread, codegen for those unordered atomic intrinsics is poor at this time. For seL4 userspace, I've implemented both [6][7].

Abstracting over memory access operations also yields the opportunity for library users to enforce further safety at the type-level. For example, one could reasonably wish to limit accesses to types which implement the zerocopy traits [8]. My wrapper type here [9] allows an AbstractRef to be created for any type, but limits actual memory operations to only types for which reading and writing in untrusted memory is guaranteed to be free of UB in the ways explained in the zerocopy docs.

I would understand if an abstraction like this were outside of this crate's scope. However, I believe that a change like this would make this crate's great core idea available in a much wider range of circumstances, without adding much additional complexity to the crate, neither for developers nor users.

For reference, nspin#1 displays the patch implementing this design that I've been using in my own branch. For the sake of minimizing the diff, I haven't actually renamed the VolatileRef and VolatilePtr types.

[1] https://github.com/seL4/rust-sel4
[2] https://github.com/coliasgroup/volatile/tree/coliasgroup
[3] https://internals.rust-lang.org/t/loads-and-stores-to-from-outside-the-memory-model/10350
[4] rust-lang/unsafe-code-guidelines#152
[5] https://fuchsia.googlesource.com/fuchsia/+/master/src/lib/shared-buffer/src/lib.rs
[6] https://github.com/seL4/rust-sel4/blob/main/crates/sel4-externally-shared/src/ops/normal_ops.rs
[7] https://github.com/seL4/rust-sel4/blob/main/crates/sel4-externally-shared/src/ops/unordered_atomic_ops.rs
[8] https://docs.rs/zerocopy/latest/zerocopy/
[9] https://github.com/seL4/rust-sel4/blob/main/crates/sel4-externally-shared/src/ops/zerocopy_ops.rs

Description

I would like to suggest another security practice recommended by the OpenSSF Scorecard which is to hash pin dependencies to prevent dependency-confusion and typosquatting attacks.

The change would only be applied to GitHub workflows, dockerfiles and shell scripts dependencies. The volatile case would only need changes on the GitHub workflow build.yml

This means:

• Hash pinning GitHub Workflow actions.

Along with hash-pinning dependencies, I also recommend adopting dependabot or renovatebot to help keep the dependencies up to date. Both tools can update hashes and associated semantic version comments.

Together with the issue I'll also suggest a PR with the changes since they are quite simple.

Any questions or concerns just let me know.
Thanks!

Additional Context

For more informations about dependency confusion / typosquatting attacks:

• Detection of malicious packages
• Counter dependency confusion (aka substitution) attacks

For more informations about the dependency-update tools:

• Dependabot
• Renovatebot