| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFRnB2WPEQaa6X6LqMk+JTxprguakNqFXvjaUGQ0QazxKH=z4Q@mail.gmail.com>
Date: Fri, 25 Oct 2024 17:12:26 -0400
From: Alex Gaynor <alex.gaynor@...il.com>
To: "Rob Herring (Arm)" <robh@...nel.org>
Cc: Saravana Kannan <saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>, Dirk Behme <dirk.behme@...il.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC 2/3] rust: Add bindings for device properties
On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@...nel.org> wrote:
>
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> ---
> Not sure if we need the KVec variant, but I kept it as that was my first
> pass attempt. Most callers are filling in some value in a driver data
> struct. Sometimes the number of elements is not known, so the caller
> calls to get the array size, allocs the correct size buffer, and then
> reads the property again to fill in the buffer.
>
> I have not implemented a wrapper for device_property_read_string(_array)
> because that API is problematic for dynamic DT nodes. The API just
> returns pointer(s) into the raw DT data. We probably need to return a
> copy of the string(s) instead for rust.
>
> After property accessors, next up is child node accessors/iterators.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 145 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9..65717cc20a23 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -19,6 +19,7 @@
> #include <linux/pci.h>
> #include <linux/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 0c28b1e6b004..bb66a28df890 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,10 +5,14 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> + alloc::KVec,
> bindings,
> + error::{to_result, Result},
> + prelude::*,
> + str::CStr,
> types::{ARef, Opaque},
> };
> -use core::{fmt, ptr};
> +use core::{fmt, mem::size_of, ptr};
>
> #[cfg(CONFIG_PRINTK)]
> use crate::c_str;
> @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> )
> };
> }
> +
> + /// Returns if a firmware property `name` is true or false
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> + }
> +
> + /// Returns if a firmware string property `name` has match for `match_str`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + let ret = unsafe {
> + bindings::device_property_match_string(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + match_str.as_ptr() as *const i8,
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` scalar value
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(&self, name, &mut val)?;
> + Ok(val[0])
> + }
> +
This, and several of the other methods are unsound, because they can
be used to construct arbitrary types for which may not allow all bit
patterns. You can use:
https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
bound to ensure only valid types are used.
Also, instead of using mem::zeroed(), you should use MaybeUnininit
(https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
which allows you to avoid needing to zero initialize.
> + /// Returns firmware property `name` array values
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(self, name, &mut val)?;
> + Ok(val)
> + }
> +
> + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
> + match size_of::<T>() {
> + 1 => to_result(unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u8,
> + val.len(),
> + )
> + })?,
> + 2 => to_result(unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u16,
> + val.len(),
> + )
> + })?,
> + 4 => to_result(unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u32,
> + val.len(),
> + )
> + })?,
> + 8 => to_result(unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u64,
> + val.len(),
> + )
> + })?,
> + _ => return Err(EINVAL),
> + }
> + Ok(())
> + }
> +
> + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: len always matches capacity
> + unsafe { val.set_len(len) }
> + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> + Ok(val)
> + }
> +
> + /// Returns array length for firmware property `name`
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> + let ret;
> +
> + match size_of::<T>() {
> + 1 => {
> + ret = unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 2 => {
> + ret = unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 4 => {
> + ret = unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 8 => {
> + ret = unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + _ => return Err(EINVAL),
> + }
> + to_result(ret)?;
> + Ok(ret.try_into().unwrap())
> + }
> }
>
> // SAFETY: Instances of `Device` are always reference-counted.
>
> --
> 2.45.2
>
--
All that is necessary for evil to succeed is for good people to do nothing.
Powered by blists - more mailing lists