Skip to content

Commit 62c7e83

Browse files
rvolosatovsrvolosatovs
authored
[35.0.0] backport fd_renumber fixes (#11281)
* fix(wasip1): prevent duplicate FD usage The implementation assumed that only the runtime could ever issue FDs, however that's not the case in p1, where guests can choose arbitrary FDs to use (e.g. via `fd_renumber`). Due to incorrect accounting, guests could "mark" arbitrary FDs as "free" and trigger a panic in the host by requesting a new FD. Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * test(wasip1): expand `fd_renumber` test Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * refactor(wasip1): do not modify descriptors on `fd_renumber(n, n)` Since `remove` is now only used once, remove it. As a sideffect, this makes the implementation more explicit . Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * fix(wasip1-adapter): prevent `unreachable` panic on `fd_renumber` Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * doc: add release notes Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * doc: reference the CVE prtest:full Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> * doc: add PR reference prtest:full Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net> --------- Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
1 parent 8f7eeb0 commit 62c7e83

File tree

4 files changed

+93
-55
lines changed

4 files changed

+93
-55
lines changed

‎RELEASES.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,12 @@ Unreleased.
163163
* An instance of `gdb` crashing on DWARF emitted by Wasmtime has been fixed.
164164
[#11077](https://github.com/bytecodealliance/wasmtime/pull/11077)
165165

166+
* Fix a panic in the host caused by preview1 guests using `fd_renumber`.
167+
[CVE-2025-53901](https://github.com/bytecodealliance/wasmtime/security/advisories/GHSA-fm79-3f68-h2fc).
168+
169+
* Fix a panic in the preview1 adapter caused by guests using `fd_renumber`.
170+
[#11277](https://github.com/bytecodealliance/wasmtime/pull/11277)
171+
166172
--------------------------------------------------------------------------------
167173

168174
Release notes for previous releases of Wasmtime can be found on the respective

‎crates/test-programs/src/bin/preview1_renumber.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,46 @@ unsafe fn test_renumber(dir_fd: wasip1::Fd) {
7474
);
7575

7676
wasip1::fd_close(fd_to).expect("closing a file");
77+
78+
wasip1::fd_renumber(0, 0).expect("renumbering 0 to 0");
79+
let fd_file3 = wasip1::path_open(
80+
dir_fd,
81+
0,
82+
"file3",
83+
wasip1::OFLAGS_CREAT,
84+
wasip1::RIGHTS_FD_READ | wasip1::RIGHTS_FD_WRITE,
85+
0,
86+
0,
87+
)
88+
.expect("opening a file");
89+
assert!(
90+
fd_file3 > libc::STDERR_FILENO as wasip1::Fd,
91+
"file descriptor range check",
92+
);
93+
94+
wasip1::fd_renumber(fd_file3, 127).expect("renumbering FD to 127");
95+
match wasip1::fd_renumber(127, u32::MAX) {
96+
Err(wasip1::ERRNO_NOMEM) => {
97+
// The preview1 adapter cannot handle more than 128 descriptors
98+
eprintln!("fd_renumber({fd_file3}, {}) returned NOMEM", u32::MAX)
99+
}
100+
res => res.expect("renumbering FD to `u32::MAX`"),
101+
}
102+
103+
let fd_file4 = wasip1::path_open(
104+
dir_fd,
105+
0,
106+
"file4",
107+
wasip1::OFLAGS_CREAT,
108+
wasip1::RIGHTS_FD_READ | wasip1::RIGHTS_FD_WRITE,
109+
0,
110+
0,
111+
)
112+
.expect("opening a file");
113+
assert!(
114+
fd_file4 > libc::STDERR_FILENO as wasip1::Fd,
115+
"file descriptor range check",
116+
);
77117
}
78118

79119
fn main() {

‎crates/wasi-preview1-component-adapter/src/descriptors.rs

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -330,8 +330,8 @@ impl Descriptors {
330330
.ok_or(wasi::ERRNO_BADF)
331331
}
332332

333-
// Internal: close a fd, returning the descriptor.
334-
fn close_(&mut self, fd: Fd) -> Result<Descriptor, Errno> {
333+
// Close an fd.
334+
pub fn close(&mut self, fd: Fd) -> Result<(), Errno> {
335335
// Throw an error if closing an fd which is already closed
336336
match self.get(fd)? {
337337
Descriptor::Closed(_) => Err(wasi::ERRNO_BADF)?,
@@ -341,12 +341,7 @@ impl Descriptors {
341341
let last_closed = self.closed;
342342
let prev = std::mem::replace(self.get_mut(fd)?, Descriptor::Closed(last_closed));
343343
self.closed = Some(fd);
344-
Ok(prev)
345-
}
346-
347-
// Close an fd.
348-
pub fn close(&mut self, fd: Fd) -> Result<(), Errno> {
349-
drop(self.close_(fd)?);
344+
drop(prev);
350345
Ok(())
351346
}
352347

@@ -366,11 +361,20 @@ impl Descriptors {
366361
while self.table_len.get() as u32 <= to_fd {
367362
self.push_closed()?;
368363
}
369-
// Then, close from_fd and put its contents into to_fd:
370-
let desc = self.close_(from_fd)?;
371-
// TODO FIXME if this overwrites a preopen, do we need to clear it from the preopen table?
372-
*self.get_mut(to_fd)? = desc;
373-
364+
// Throw an error if renumbering a closed fd
365+
match self.get(from_fd)? {
366+
Descriptor::Closed(_) => Err(wasi::ERRNO_BADF)?,
367+
_ => {}
368+
}
369+
// Close from_fd and put its contents into to_fd
370+
if from_fd != to_fd {
371+
// Mutate the descriptor to be closed, and push the closed fd onto the head of the linked list:
372+
let last_closed = self.closed;
373+
let desc = std::mem::replace(self.get_mut(from_fd)?, Descriptor::Closed(last_closed));
374+
self.closed = Some(from_fd);
375+
// TODO FIXME if this overwrites a preopen, do we need to clear it from the preopen table?
376+
*self.get_mut(to_fd)? = desc;
377+
}
374378
Ok(())
375379
}
376380

‎crates/wasi/src/preview1.rs

Lines changed: 30 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,8 @@ use crate::p2::bindings::{
7474
};
7575
use crate::p2::{FsError, IsATTY, WasiCtx, WasiImpl, WasiView};
7676
use anyhow::{Context, bail};
77-
use std::collections::{BTreeMap, HashSet};
77+
use std::collections::{BTreeMap, BTreeSet, HashSet, btree_map};
7878
use std::mem::{self, size_of, size_of_val};
79-
use std::ops::{Deref, DerefMut};
8079
use std::slice;
8180
use std::sync::Arc;
8281
use std::sync::atomic::{AtomicU64, Ordering};
@@ -286,21 +285,7 @@ struct WasiPreview1Adapter {
286285
#[derive(Debug, Default)]
287286
struct Descriptors {
288287
used: BTreeMap<u32, Descriptor>,
289-
free: Vec<u32>,
290-
}
291-
292-
impl Deref for Descriptors {
293-
type Target = BTreeMap<u32, Descriptor>;
294-
295-
fn deref(&self) -> &Self::Target {
296-
&self.used
297-
}
298-
}
299-
300-
impl DerefMut for Descriptors {
301-
fn deref_mut(&mut self) -> &mut Self::Target {
302-
&mut self.used
303-
}
288+
free: BTreeSet<u32>,
304289
}
305290

306291
impl Descriptors {
@@ -377,42 +362,34 @@ impl Descriptors {
377362

378363
/// Returns next descriptor number, which was never assigned
379364
fn unused(&self) -> Result<u32> {
380-
match self.last_key_value() {
365+
match self.used.last_key_value() {
381366
Some((fd, _)) => {
382367
if let Some(fd) = fd.checked_add(1) {
383368
return Ok(fd);
384369
}
385-
if self.len() == u32::MAX as usize {
370+
if self.used.len() == u32::MAX as usize {
386371
return Err(types::Errno::Loop.into());
387372
}
388373
// TODO: Optimize
389374
Ok((0..u32::MAX)
390375
.rev()
391-
.find(|fd| !self.contains_key(fd))
376+
.find(|fd| !self.used.contains_key(fd))
392377
.expect("failed to find an unused file descriptor"))
393378
}
394379
None => Ok(0),
395380
}
396381
}
397382

398-
/// Removes the [Descriptor] corresponding to `fd`
399-
fn remove(&mut self, fd: types::Fd) -> Option<Descriptor> {
400-
let fd = fd.into();
401-
let desc = self.used.remove(&fd)?;
402-
self.free.push(fd);
403-
Some(desc)
404-
}
405-
406383
/// Pushes the [Descriptor] returning corresponding number.
407384
/// This operation will try to reuse numbers previously removed via [`Self::remove`]
408385
/// and rely on [`Self::unused`] if no free numbers are recorded
409386
fn push(&mut self, desc: Descriptor) -> Result<u32> {
410-
let fd = if let Some(fd) = self.free.pop() {
387+
let fd = if let Some(fd) = self.free.pop_last() {
411388
fd
412389
} else {
413390
self.unused()?
414391
};
415-
assert!(self.insert(fd, desc).is_none());
392+
assert!(self.used.insert(fd, desc).is_none());
416393
Ok(fd)
417394
}
418395
}
@@ -453,15 +430,15 @@ impl Transaction<'_> {
453430
/// Returns [`types::Errno::Badf`] if no [`Descriptor`] is found
454431
fn get_descriptor(&self, fd: types::Fd) -> Result<&Descriptor> {
455432
let fd = fd.into();
456-
let desc = self.descriptors.get(&fd).ok_or(types::Errno::Badf)?;
433+
let desc = self.descriptors.used.get(&fd).ok_or(types::Errno::Badf)?;
457434
Ok(desc)
458435
}
459436

460437
/// Borrows [`File`] corresponding to `fd`
461438
/// if it describes a [`Descriptor::File`]
462439
fn get_file(&self, fd: types::Fd) -> Result<&File> {
463440
let fd = fd.into();
464-
match self.descriptors.get(&fd) {
441+
match self.descriptors.used.get(&fd) {
465442
Some(Descriptor::File(file)) => Ok(file),
466443
_ => Err(types::Errno::Badf.into()),
467444
}
@@ -471,7 +448,7 @@ impl Transaction<'_> {
471448
/// if it describes a [`Descriptor::File`]
472449
fn get_file_mut(&mut self, fd: types::Fd) -> Result<&mut File> {
473450
let fd = fd.into();
474-
match self.descriptors.get_mut(&fd) {
451+
match self.descriptors.used.get_mut(&fd) {
475452
Some(Descriptor::File(file)) => Ok(file),
476453
_ => Err(types::Errno::Badf.into()),
477454
}
@@ -485,7 +462,7 @@ impl Transaction<'_> {
485462
/// Returns [`types::Errno::Spipe`] if the descriptor corresponds to stdio
486463
fn get_seekable(&self, fd: types::Fd) -> Result<&File> {
487464
let fd = fd.into();
488-
match self.descriptors.get(&fd) {
465+
match self.descriptors.used.get(&fd) {
489466
Some(Descriptor::File(file)) => Ok(file),
490467
Some(
491468
Descriptor::Stdin { .. } | Descriptor::Stdout { .. } | Descriptor::Stderr { .. },
@@ -518,7 +495,7 @@ impl Transaction<'_> {
518495
/// if it describes a [`Descriptor::Directory`]
519496
fn get_dir_fd(&self, fd: types::Fd) -> Result<Resource<filesystem::Descriptor>> {
520497
let fd = fd.into();
521-
match self.descriptors.get(&fd) {
498+
match self.descriptors.used.get(&fd) {
522499
Some(Descriptor::Directory { fd, .. }) => Ok(fd.borrowed()),
523500
_ => Err(types::Errno::Badf.into()),
524501
}
@@ -1327,11 +1304,13 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiP1Ctx {
13271304
_memory: &mut GuestMemory<'_>,
13281305
fd: types::Fd,
13291306
) -> Result<(), types::Error> {
1330-
let desc = self
1331-
.transact()?
1332-
.descriptors
1333-
.remove(fd)
1334-
.ok_or(types::Errno::Badf)?;
1307+
let desc = {
1308+
let fd = fd.into();
1309+
let mut st = self.transact()?;
1310+
let desc = st.descriptors.used.remove(&fd).ok_or(types::Errno::Badf)?;
1311+
st.descriptors.free.insert(fd);
1312+
desc
1313+
};
13351314
match desc {
13361315
Descriptor::Stdin { stream, .. } => {
13371316
streams::HostInputStream::drop(&mut self.as_io_impl(), stream)
@@ -1830,8 +1809,17 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiP1Ctx {
18301809
to: types::Fd,
18311810
) -> Result<(), types::Error> {
18321811
let mut st = self.transact()?;
1833-
let desc = st.descriptors.remove(from).ok_or(types::Errno::Badf)?;
1834-
st.descriptors.insert(to.into(), desc);
1812+
let from = from.into();
1813+
let btree_map::Entry::Occupied(desc) = st.descriptors.used.entry(from) else {
1814+
return Err(types::Errno::Badf.into());
1815+
};
1816+
let to = to.into();
1817+
if from != to {
1818+
let desc = desc.remove();
1819+
st.descriptors.free.insert(from);
1820+
st.descriptors.free.remove(&to);
1821+
st.descriptors.used.insert(to, desc);
1822+
}
18351823
Ok(())
18361824
}
18371825

0 commit comments

Comments
 (0)