diff --git a/kernel/src/syscall/preadv.rs b/kernel/src/syscall/preadv.rs index 8ffe35d95..af9e4e464 100644 --- a/kernel/src/syscall/preadv.rs +++ b/kernel/src/syscall/preadv.rs @@ -4,7 +4,7 @@ use super::SyscallReturn; use crate::{ fs::file_table::{get_file_fast, FileDesc}, prelude::*, - util::{MultiWrite, VmWriterArray}, + util::VmWriterArray, }; pub fn sys_readv( @@ -78,24 +78,7 @@ fn do_sys_preadv( let user_space = ctx.user_space(); let mut writer_array = VmWriterArray::from_user_io_vecs(&user_space, io_vec_ptr, io_vec_count)?; for writer in writer_array.writers_mut() { - if !writer.has_avail() { - continue; - } - - let writer_len = writer.sum_lens(); - if total_len.checked_add(writer_len).is_none() - || total_len - .checked_add(writer_len) - .and_then(|sum| sum.checked_add(cur_offset)) - .is_none() - || total_len - .checked_add(writer_len) - .and_then(|sum| sum.checked_add(cur_offset)) - .map(|sum| sum > isize::MAX as usize) - .unwrap_or(false) - { - return_errno_with_message!(Errno::EINVAL, "Total length overflow"); - } + debug_assert!(writer.has_avail()); // TODO: According to the man page // at , @@ -143,9 +126,7 @@ fn do_sys_readv( let user_space = ctx.user_space(); let mut writer_array = VmWriterArray::from_user_io_vecs(&user_space, io_vec_ptr, io_vec_count)?; for writer in writer_array.writers_mut() { - if !writer.has_avail() { - continue; - } + debug_assert!(writer.has_avail()); // TODO: According to the man page // at , diff --git a/kernel/src/syscall/pwritev.rs b/kernel/src/syscall/pwritev.rs index e75332f80..d906ca76b 100644 --- a/kernel/src/syscall/pwritev.rs +++ b/kernel/src/syscall/pwritev.rs @@ -80,24 +80,7 @@ fn do_sys_pwritev( let user_space = ctx.user_space(); let mut reader_array = VmReaderArray::from_user_io_vecs(&user_space, io_vec_ptr, io_vec_count)?; for reader in reader_array.readers_mut() { - if !reader.has_remain() { - continue; - } - - let reader_len = reader.remain(); - if total_len.checked_add(reader_len).is_none() - || total_len - .checked_add(reader_len) - .and_then(|sum| sum.checked_add(cur_offset)) - .is_none() - || total_len - .checked_add(reader_len) - .and_then(|sum| sum.checked_add(cur_offset)) - .map(|sum| sum > isize::MAX as usize) - .unwrap_or(false) - { - return_errno_with_message!(Errno::EINVAL, "Total length overflow"); - } + debug_assert!(reader.has_remain()); // TODO: According to the man page // at , @@ -140,9 +123,7 @@ fn do_sys_writev( let user_space = ctx.user_space(); let mut reader_array = VmReaderArray::from_user_io_vecs(&user_space, io_vec_ptr, io_vec_count)?; for reader in reader_array.readers_mut() { - if !reader.has_remain() { - continue; - } + debug_assert!(reader.has_remain()); // TODO: According to the man page // at , diff --git a/kernel/src/util/iovec.rs b/kernel/src/util/iovec.rs index 8fdee645e..ca3243ddd 100644 --- a/kernel/src/util/iovec.rs +++ b/kernel/src/util/iovec.rs @@ -53,6 +53,25 @@ impl IoVec { } } +/// The maximum number of buffers in the I/O vector. +/// +/// Reference: . +pub(super) const MAX_IO_VECTOR_LENGTH: usize = 1024; +/// The maximum bytes of all buffers in the I/O vector. +/// +/// According to man pages, the kernel should fail with [`Errno::EINVAL`] if the number of bytes in +/// the I/O vector exceeds this threshold. See +/// . +/// +/// However, the actual Linux behavior is to truncate the buffer and ignore the remaining buffer +/// space. See . +/// +/// Typical 64-bit architectures do not have 64-bit virtual address space, and the value of +/// [`MAX_IO_VECTOR_LENGTH`] is relatively small. Therefore, userspace may not be able to supply a +/// valid I/O vector containing so many bytes. Nevertheless, we should still check against this to +/// prevent overflows in the future, e.g., when the virtual address space becomes larger. +const MAX_TOTAL_IOV_BYTES: usize = isize::MAX as usize; + /// The util function for create [`VmReader`]/[`VmWriter`]s. fn copy_iovs_and_convert<'a, T: 'a>( user_space: &'a CurrentUserSpace<'a>, @@ -60,11 +79,17 @@ fn copy_iovs_and_convert<'a, T: 'a>( count: usize, convert_iovec: impl Fn(&IoVec, &'a VmSpace) -> Result, ) -> Result> { + if count > MAX_IO_VECTOR_LENGTH { + return_errno_with_message!(Errno::EINVAL, "the I/O vector contains too many buffers"); + } + let vm_space = user_space.root_vmar().vm_space(); let mut v = Vec::with_capacity(count); + let mut max_len = MAX_TOTAL_IOV_BYTES; + for idx in 0..count { - let iov = { + let mut iov = { let addr = start_addr + idx * core::mem::size_of::(); let uiov: UserIoVec = vm_space .reader(addr, core::mem::size_of::())? @@ -72,6 +97,13 @@ fn copy_iovs_and_convert<'a, T: 'a>( IoVec::try_from(uiov)? }; + // Truncate the buffer if the number of bytes exceeds `MAX_TOTAL_IOV_BYTES`. + // See comments above the `MAX_TOTAL_IOV_BYTES` constant for more details. + if iov.len > max_len { + iov.len = max_len; + } + max_len -= iov.len; + if iov.is_empty() { continue; } @@ -94,7 +126,10 @@ pub struct VmReaderArray<'a>(Box<[VmReader<'a>]>); pub struct VmWriterArray<'a>(Box<[VmWriter<'a>]>); impl<'a> VmReaderArray<'a> { - /// Creates a new `VmReaderArray` from user-provided io vec buffer. + /// Creates a new `VmReaderArray` from user-provided I/O vector buffers. + /// + /// This ensures that empty buffers are filtered out, meaning that all of the returned readers + /// should be non-empty. pub fn from_user_io_vecs( user_space: &'a CurrentUserSpace<'a>, start_addr: Vaddr, @@ -117,7 +152,10 @@ impl<'a> VmReaderArray<'a> { } impl<'a> VmWriterArray<'a> { - /// Creates a new `VmWriterArray` from user-provided io vec buffer. + /// Creates a new `VmWriterArray` from user-provided I/O vector buffers. + /// + /// This ensures that empty buffers are filtered out, meaning that all of the returned writers + /// should be non-empty. pub fn from_user_io_vecs( user_space: &'a CurrentUserSpace<'a>, start_addr: Vaddr, diff --git a/kernel/src/util/net/socket.rs b/kernel/src/util/net/socket.rs index 2217edef5..db0ff5de1 100644 --- a/kernel/src/util/net/socket.rs +++ b/kernel/src/util/net/socket.rs @@ -4,7 +4,10 @@ use super::read_socket_addr_from_user; use crate::{ net::socket::util::{ControlMessage, SocketAddr}, prelude::*, - util::{net::write_socket_addr_with_max_len, VmReaderArray, VmWriterArray}, + util::{ + iovec::MAX_IO_VECTOR_LENGTH, net::write_socket_addr_with_max_len, VmReaderArray, + VmWriterArray, + }, }; /// Standard well-defined IP protocols. @@ -154,6 +157,10 @@ impl CUserMsgHdr { &self, user_space: &'a CurrentUserSpace<'a>, ) -> Result> { + if self.msg_iovlen as usize > MAX_IO_VECTOR_LENGTH { + return_errno_with_message!(Errno::EMSGSIZE, "the I/O vector contains too many buffers"); + } + VmReaderArray::from_user_io_vecs(user_space, self.msg_iov, self.msg_iovlen as usize) } @@ -161,6 +168,10 @@ impl CUserMsgHdr { &self, user_space: &'a CurrentUserSpace<'a>, ) -> Result> { + if self.msg_iovlen as usize > MAX_IO_VECTOR_LENGTH { + return_errno_with_message!(Errno::EMSGSIZE, "the I/O vector contains too many buffers"); + } + VmWriterArray::from_user_io_vecs(user_space, self.msg_iov, self.msg_iovlen as usize) } } diff --git a/test/src/apps/file_io/iovec_err.c b/test/src/apps/file_io/iovec_err.c new file mode 100644 index 000000000..530919a4e --- /dev/null +++ b/test/src/apps/file_io/iovec_err.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: MPL-2.0 + +#include +#include +#include +#include +#include + +#include "../test.h" + +static char buf[16]; +static struct iovec iov_long[UIO_MAXIOV + 2]; +static struct iovec iov_inv[2]; + +FN_SETUP(iov) +{ + int i; + + for (i = 0; i <= UIO_MAXIOV + 1; ++i) + iov_long[i] = (struct iovec){ .iov_base = buf, + .iov_len = sizeof(buf) }; + + iov_inv[0] = (struct iovec){ .iov_base = buf, .iov_len = sizeof(buf) }; + iov_inv[1] = (struct iovec){ .iov_base = buf, .iov_len = -1234 }; +} +END_SETUP() + +FN_TEST(readv) +{ + int fd; + + fd = TEST_SUCC(open("/dev/zero", O_RDONLY)); + + TEST_SUCC(readv(fd, iov_long, UIO_MAXIOV - 1)); + TEST_SUCC(readv(fd, iov_long, UIO_MAXIOV)); + TEST_ERRNO(readv(fd, iov_long, UIO_MAXIOV + 1), EINVAL); + + TEST_SUCC(readv(fd, iov_inv, 1)); + TEST_ERRNO(readv(fd, iov_inv, 2), EINVAL); + + TEST_SUCC(close(fd)); +} +END_TEST() + +FN_TEST(writev) +{ + int fd; + + fd = TEST_SUCC(open("/dev/null", O_WRONLY)); + + TEST_SUCC(writev(fd, iov_long, UIO_MAXIOV - 1)); + TEST_SUCC(writev(fd, iov_long, UIO_MAXIOV)); + TEST_ERRNO(writev(fd, iov_long, UIO_MAXIOV + 1), EINVAL); + + TEST_SUCC(writev(fd, iov_inv, 1)); + TEST_ERRNO(writev(fd, iov_inv, 2), EINVAL); + + TEST_SUCC(close(fd)); +} +END_TEST() + +FN_TEST(sendmsg_recvmsg) +{ + int fds[2]; + struct msghdr msgh; + + TEST_SUCC(socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds)); + + memset(&msgh, 0, sizeof(msgh)); +#define sendv(fd, iov, iovcnt) \ + ({ \ + msgh.msg_iov = iov; \ + msgh.msg_iovlen = iovcnt; \ + sendmsg(fd, &msgh, 0); \ + }) +#define recvv(fd, iov, iovcnt) \ + ({ \ + msgh.msg_iov = iov; \ + msgh.msg_iovlen = iovcnt; \ + recvmsg(fd, &msgh, 0); \ + }) + + TEST_SUCC(sendv(fds[0], iov_long, UIO_MAXIOV - 1)); + TEST_SUCC(sendv(fds[0], iov_long, UIO_MAXIOV)); + TEST_ERRNO(sendv(fds[0], iov_long, UIO_MAXIOV + 1), EMSGSIZE); + + TEST_SUCC(recvv(fds[1], iov_long, UIO_MAXIOV - 1)); + TEST_SUCC(recvv(fds[1], iov_long, UIO_MAXIOV)); + TEST_ERRNO(recvv(fds[1], iov_long, UIO_MAXIOV + 1), EMSGSIZE); + + TEST_SUCC(sendv(fds[0], iov_inv, 1)); + TEST_ERRNO(sendv(fds[0], iov_inv, 2), EINVAL); + + TEST_SUCC(recvv(fds[1], iov_inv, 1)); + TEST_ERRNO(recvv(fds[1], iov_inv, 2), EINVAL); + +#undef sendv +#undef recvv + + TEST_SUCC(close(fds[0])); + TEST_SUCC(close(fds[1])); +} +END_TEST() diff --git a/test/src/apps/scripts/fs.sh b/test/src/apps/scripts/fs.sh index 3df1597f3..51db78022 100755 --- a/test/src/apps/scripts/fs.sh +++ b/test/src/apps/scripts/fs.sh @@ -65,3 +65,4 @@ pipe/pipe_err pipe/short_rw epoll/epoll_err epoll/poll_err +file_io/iovec_err