From 4200e2f1dc18e365029f2aa5baae0c43701f8dcd Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sat, 14 Dec 2024 13:48:57 -0800 Subject: [PATCH] kern: reply buffer capacity checking The syscall docs say that attempting to reply with a message that's too big for your client to accept is a programming error. Programming errors are expected to produce faults in the program with the error. This fault is documented as being of type `ReplyTooLarge`. However, there is no such fault defined in the ABI, and the actual behavior of the kernel is to deliver the prefix of the reply that fits, and hand the client the length of that prefix. In our current world, this means the client will generally die shortly thereafter with a deserialization error. In a potential future world where messages can contain variable length portions, the client might not even hit an error when the reply is truncated. The code in the kernel has been marked with a TODO since forever. This commit resolves the TODO by adding a check and fault reason. --- doc/syscalls.adoc | 8 +++++--- sys/abi/src/lib.rs | 4 ++++ sys/kern/src/syscalls.rs | 13 +++++++++---- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/doc/syscalls.adoc b/doc/syscalls.adoc index a353b7674..2389d2567 100644 --- a/doc/syscalls.adoc +++ b/doc/syscalls.adoc @@ -332,7 +332,7 @@ There is only one way to break `REPLY`, and that's with a bogus slice. | `MemoryAccess` | Reply message is longer than recipient requested. -| `ReplyTooLarge` +| `ReplyTooBig` |=== @@ -346,8 +346,10 @@ Reply messages can be zero-length, in which case the base address of the slice is ignored. Often, the response code is enough. `RECV` delivers the size of the caller's response buffer, so your task has -sufficient information to not overflow it. This is why doing so is a fault: it's -a programming error. +sufficient information to not overflow it. If the caller's response buffer is +too small, you are expected to instead use `REPLY_FAULT` with the +`ReplyBufferTooSmall` code. If you instead send a reply that won't fit, that's +treated as a programming error in your task, and you take a fault. [#sys_set_timer] === `SET_TIMER` (3) diff --git a/sys/abi/src/lib.rs b/sys/abi/src/lib.rs index dd00782e4..c26eb8cb0 100644 --- a/sys/abi/src/lib.rs +++ b/sys/abi/src/lib.rs @@ -386,6 +386,10 @@ pub enum UsageError { BadKernelMessage, BadReplyFaultReason, NotSupervisor, + + // A server is attempting to reply with a message that is too large for the + // client to handle. + ReplyTooBig, } /// Origin of a fault. diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index 52517ef63..a1d16d777 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -419,11 +419,16 @@ fn reply(tasks: &mut [Task], caller: usize) -> Result { } }; + // Validate that the reply fits in the recipient's buffer. Servers are + // expected to get this right, because the reply size is delivered along + // with the message: if the client didn't provide enough space, the server + // should reply-fault instead of replying. So, we assume any reply that + // would be truncated is a server bug. + if src_slice.len() > dest_slice.len() { + return Err(FaultInfo::SyscallUsage(UsageError::ReplyTooBig)); + } + // Okay, ready to attempt the copy. - // TODO: we want to treat any attempt to copy more than will fit as a fault - // in the task that is replying, because it knows how big the target buffer - // is and is expected to respect that. This is not currently implemented -- - // currently you'll get the prefix. let amount_copied = safe_copy(tasks, caller, src_slice, callee, dest_slice); let amount_copied = match amount_copied { Ok(n) => n,