| 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: <20240913112643.542914-4-benno.lossin@proton.me>
Date: Fri, 13 Sep 2024 11:27:05 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Greg KH <gregkh@...uxfoundation.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, 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@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: [RFC PATCH 3/3] WIP: rust: tarfs: use untrusted data API
As an example here is how tarfs could use the untrusted data API. There
are two calls to `untrusted()` outside of `Validator` implementations. I
don't know if the names are used for some logic at a later point, and I
didn't want to mark the name as untrusted, because then I would have to
change more of the vfs API. I think if the names are not used for any
logic, then they can just stay as `Untrusted<CString>`.
---
fs/tarfs/defs.rs | 1 +
fs/tarfs/tar.rs | 143 ++++++++++++++++++++++++++++++++---------------
2 files changed, 99 insertions(+), 45 deletions(-)
diff --git a/fs/tarfs/defs.rs b/fs/tarfs/defs.rs
index 7481b75aaab2..db22e3299fef 100644
--- a/fs/tarfs/defs.rs
+++ b/fs/tarfs/defs.rs
@@ -70,6 +70,7 @@ pub struct DirEntry {
/// The super-block of a tarfs instance.
#[repr(C)]
+ #[derive(Clone)]
pub struct Header {
/// The offset to the beginning of the inode-table.
pub inode_table_offset: LE<u64>,
diff --git a/fs/tarfs/tar.rs b/fs/tarfs/tar.rs
index a3f6e468e566..8ca255306c8b 100644
--- a/fs/tarfs/tar.rs
+++ b/fs/tarfs/tar.rs
@@ -9,7 +9,14 @@
inode::Type, iomap, sb, sb::SuperBlock, Offset, Stat,
};
use kernel::types::{ARef, Either, FromBytes, Locked};
-use kernel::{c_str, prelude::*, str::CString, user};
+use kernel::{
+ c_str,
+ prelude::*,
+ str::CString,
+ types::{Untrusted, Validator},
+ user,
+};
+use std::fs::DirEntry;
pub mod defs;
@@ -29,7 +36,8 @@
static_assert!(SECTORS_PER_BLOCK > 0);
-struct INodeData {
+#[allow(missing_docs)]
+pub struct INodeData {
offset: u64,
flags: u8,
}
@@ -41,26 +49,13 @@ struct TarFs {
mapper: inode::Mapper,
}
-impl TarFs {
- fn iget(sb: &SuperBlock<Self>, ino: u64) -> Result<ARef<INode<Self>>> {
- // Check that the inode number is valid.
- let h = sb.data();
- if ino == 0 || ino > h.inode_count {
- return Err(ENOENT);
- }
+impl Validator for Inode {
+ type Input = [u8];
+ type Output = inode::Params<INodeData>;
+ type Err = Error;
- // Create an inode or find an existing (cached) one.
- let mut inode = match sb.get_or_create_inode(ino)? {
- Either::Left(existing) => return Ok(existing),
- Either::Right(new) => new,
- };
-
- static_assert!((TARFS_BSIZE as usize) % size_of::<Inode>() == 0);
-
- // Load inode details from storage.
- let offset = h.inode_table_offset + (ino - 1) * u64::try_from(size_of::<Inode>())?;
- let b = h.mapper.mapped_folio(offset.try_into()?)?;
- let idata = Inode::from_bytes(&b, 0).ok_or(EIO)?;
+ fn validate(untrusted: &Untrusted<Self::Input>) -> Result<Self::Output, Self::Err> {
+ let idata = Inode::from_bytes(untrusted.untrusted(), 0).ok_or(EIO)?;
let mode = idata.mode.value();
@@ -69,36 +64,21 @@ fn iget(sb: &SuperBlock<Self>, ino: u64) -> Result<ARef<INode<Self>>> {
return Err(ENOENT);
}
- const DIR_FOPS: file::Ops<TarFs> = file::Ops::new::<TarFs>();
- const DIR_IOPS: inode::Ops<TarFs> = inode::Ops::new::<TarFs>();
- const FILE_AOPS: address_space::Ops<TarFs> = iomap::ro_aops::<TarFs>();
-
let size = idata.size.value();
let doffset = idata.offset.value();
let secs = u64::from(idata.lmtime.value()) | (u64::from(idata.hmtime & 0xf) << 32);
let ts = kernel::time::Timespec::new(secs, 0)?;
let typ = match mode & fs::mode::S_IFMT {
- fs::mode::S_IFREG => {
- inode
- .set_fops(file::Ops::generic_ro_file())
- .set_aops(FILE_AOPS);
- Type::Reg
- }
- fs::mode::S_IFDIR => {
- inode.set_iops(DIR_IOPS).set_fops(DIR_FOPS);
- Type::Dir
- }
- fs::mode::S_IFLNK => {
- inode.set_iops(inode::Ops::simple_symlink_inode());
- Type::Lnk(Some(Self::get_link(sb, doffset, size)?))
- }
+ fs::mode::S_IFREG => Type::Reg,
+ fs::mode::S_IFDIR => Type::Dir,
+ fs::mode::S_IFLNK => Type::Lnk(None),
fs::mode::S_IFSOCK => Type::Sock,
fs::mode::S_IFIFO => Type::Fifo,
fs::mode::S_IFCHR => Type::Chr((doffset >> 32) as u32, doffset as u32),
fs::mode::S_IFBLK => Type::Blk((doffset >> 32) as u32, doffset as u32),
_ => return Err(ENOENT),
};
- inode.init(inode::Params {
+ Ok(inode::Params {
typ,
mode: mode & 0o777,
size: size.try_into()?,
@@ -115,13 +95,86 @@ fn iget(sb: &SuperBlock<Self>, ino: u64) -> Result<ARef<INode<Self>>> {
},
})
}
+}
+
+impl Validator for Header {
+ type Input = [u8];
+ type Output = Self;
+ type Err = Error;
+
+ fn validate(
+ untrusted: &Untrusted<Self::Input>,
+ ) -> core::result::Result<Self::Output, Self::Err> {
+ Header::from_bytes(untrusted.untrusted(), 0)
+ .ok_or(EIO)
+ .cloned()
+ }
+}
+
+impl Validator for DirEntry {
+ type Input = [u8];
+ type Output = Self;
+ type Err = Error;
+
+ fn validate(
+ untrusted: &Untrusted<Self::Input>,
+ ) -> core::result::Result<Self::Output, Self::Err> {
+ let raw = DirEntry::from_bytes_to_slice(untrusted.untrusted()).ok_or(EIO)?;
+ // TODO: actually verify that the `raw` entry is correct.
+ Ok(raw)
+ }
+}
+
+impl TarFs {
+ fn iget(sb: &SuperBlock<Self>, ino: u64) -> Result<ARef<INode<Self>>> {
+ // Check that the inode number is valid.
+ let h = sb.data();
+ if ino == 0 || ino > h.inode_count {
+ return Err(ENOENT);
+ }
+
+ // Create an inode or find an existing (cached) one.
+ let mut inode = match sb.get_or_create_inode(ino)? {
+ Either::Left(existing) => return Ok(existing),
+ Either::Right(new) => new,
+ };
+
+ static_assert!((TARFS_BSIZE as usize) % size_of::<Inode>() == 0);
+
+ // Load inode details from storage.
+ let offset = h.inode_table_offset + (ino - 1) * u64::try_from(size_of::<Inode>())?;
+ let b = h.mapper.mapped_folio(offset.try_into()?)?;
+
+ const DIR_FOPS: file::Ops<TarFs> = file::Ops::new::<TarFs>();
+ const DIR_IOPS: inode::Ops<TarFs> = inode::Ops::new::<TarFs>();
+ const FILE_AOPS: address_space::Ops<TarFs> = iomap::ro_aops::<TarFs>();
+
+ let mut params = b.deref().validate::<Inode>()?;
+ match ¶ms.typ {
+ Type::Reg => {
+ inode
+ .set_fops(file::Ops::generic_ro_file())
+ .set_aops(FILE_AOPS);
+ }
+ Type::Dir => {
+ inode.set_iops(DIR_IOPS).set_fops(DIR_FOPS);
+ }
+ Type::Lnk(_) => {
+ inode.set_iops(inode::Ops::simple_symlink_inode());
+ let doffset = params.value.offset;
+ params.typ = Type::Lnk(Some(Self::get_link(sb, doffset, params.size as u64)?));
+ }
+ _ => {}
+ }
+ inode.init(params)
+ }
fn name_eq(sb: &SuperBlock<Self>, mut name: &[u8], offset: u64) -> Result<bool> {
let ret =
sb.data()
.mapper
.for_each_page(offset as Offset, name.len().try_into()?, |data| {
- if data != &name[..data.len()] {
+ if data.untrusted() != &name[..data.len()] {
return Ok(Some(()));
}
name = &name[data.len()..];
@@ -135,7 +188,7 @@ fn read_name(sb: &SuperBlock<Self>, name: &mut [u8], offset: u64) -> Result {
sb.data()
.mapper
.for_each_page(offset as Offset, name.len().try_into()?, |data| {
- name[copy_to..][..data.len()].copy_from_slice(data);
+ name[copy_to..][..data.len()].copy_from_slice(data.untrusted());
copy_to += data.len();
Ok(None::<()>)
})?;
@@ -179,7 +232,7 @@ fn fill_super(
let tarfs = {
let offset = (scount - 1) * SECTOR_SIZE;
let mapped = mapper.mapped_folio(offset.try_into()?)?;
- let hdr = Header::from_bytes(&mapped, 0).ok_or(EIO)?;
+ let hdr = mapped.deref().validate::<Header>()?;
let inode_table_offset = hdr.inode_table_offset.value();
let inode_count = hdr.inode_count.value();
drop(mapped);
@@ -316,7 +369,7 @@ fn lookup(
parent.data().offset.try_into()?,
parent.size(),
|data| {
- for e in DirEntry::from_bytes_to_slice(data).ok_or(EIO)? {
+ for e in data.validate::<DirEntry>()? {
if Self::name_eq(sb, name, e.name_offset.value())? {
return Ok(Some(Self::iget(sb, e.ino.value())?));
}
@@ -368,7 +421,7 @@ fn read_dir(
inode.data().offset as i64 + pos,
inode.size() - pos,
|data| {
- for e in DirEntry::from_bytes_to_slice(data).ok_or(EIO)? {
+ for e in data.validate::<DirEntry>()? {
let name_len = usize::try_from(e.name_len.value())?;
if name_len > name.len() {
name.resize(name_len, 0, GFP_NOFS)?;
--
2.46.0
Powered by blists - more mailing lists