From fc99a20295e86e4ce21d052e05fcabcc225dbbf0 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Thu, 26 Aug 2021 15:48:15 +0000 Subject: fesvr: replace use of `std::vector::operator[0]` This replaces multiple uses of `std::vector::operator[]` where the parameter is a constant `0` with the use of C++11's `std::vector::data` method. This fixes the root cause of invalid memory accesses. `std::vector::operator[]` is an unchecked memory access, and when the buffers are zero-sized (that is the buffer container is empty) either due to a 0 padding in the case of elfloader or NULL parameters to syscalls where permitted, the unchecked access may cause an invalid memory access. The use of `std::vector::data` is permitted even in such a case, though the returned memory may not be dereferenced. The general usage of the returned pointer is to pass to `memif_t`, which is careful about 0-sized buffer accesses, and so passing the result of `std::vector::data` is safe. This is theoretically a better access pattern as it also avoids having the compiler to re-materialize the pointer from the de-referenced location. --- fesvr/device.cc | 8 +++---- fesvr/elfloader.cc | 2 +- fesvr/htif.cc | 2 +- fesvr/syscall.cc | 70 +++++++++++++++++++++++++++--------------------------- 4 files changed, 41 insertions(+), 41 deletions(-) diff --git a/fesvr/device.cc b/fesvr/device.cc index 3a4cc95..3cd3cd4 100644 --- a/fesvr/device.cc +++ b/fesvr/device.cc @@ -110,10 +110,10 @@ void disk_t::handle_read(command_t cmd) cmd.memif().read(cmd.payload(), sizeof(req), &req); std::vector buf(req.size); - if ((size_t)::pread(fd, &buf[0], buf.size(), req.offset) != req.size) + if ((size_t)::pread(fd, buf.data(), buf.size(), req.offset) != req.size) throw std::runtime_error("could not read " + id + " @ " + std::to_string(req.offset)); - cmd.memif().write(req.addr, buf.size(), &buf[0]); + cmd.memif().write(req.addr, buf.size(), buf.data()); cmd.respond(req.tag); } @@ -123,9 +123,9 @@ void disk_t::handle_write(command_t cmd) cmd.memif().read(cmd.payload(), sizeof(req), &req); std::vector buf(req.size); - cmd.memif().read(req.addr, buf.size(), &buf[0]); + cmd.memif().read(req.addr, buf.size(), buf.data()); - if ((size_t)::pwrite(fd, &buf[0], buf.size(), req.offset) != req.size) + if ((size_t)::pwrite(fd, buf.data(), buf.size(), req.offset) != req.size) throw std::runtime_error("could not write " + id + " @ " + std::to_string(req.offset)); cmd.respond(req.tag); diff --git a/fesvr/elfloader.cc b/fesvr/elfloader.cc index e5e2c6d..76cd6da 100644 --- a/fesvr/elfloader.cc +++ b/fesvr/elfloader.cc @@ -56,7 +56,7 @@ std::map load_elf(const char* fn, memif_t* memif, reg_t* if (size_t pad = bswap(ph[i].p_memsz) - bswap(ph[i].p_filesz)) { \ zeros.resize(pad); \ memif->write(bswap(ph[i].p_paddr) + bswap(ph[i].p_filesz), pad, \ - &zeros[0]); \ + zeros.data()); \ } \ } \ } \ diff --git a/fesvr/htif.cc b/fesvr/htif.cc index 1a9d0fc..234032b 100644 --- a/fesvr/htif.cc +++ b/fesvr/htif.cc @@ -169,7 +169,7 @@ void htif_t::stop() if (!sig_file.empty() && sig_len) // print final torture test signature { std::vector buf(sig_len); - mem.read(sig_addr, sig_len, &buf[0]); + mem.read(sig_addr, sig_len, buf.data()); std::ofstream sigs(sig_file); assert(sigs && "can't open signature file!"); diff --git a/fesvr/syscall.cc b/fesvr/syscall.cc index 298d851..ab7fc3b 100644 --- a/fesvr/syscall.cc +++ b/fesvr/syscall.cc @@ -220,36 +220,36 @@ static reg_t sysret_errno(sreg_t ret) reg_t syscall_t::sys_read(reg_t fd, reg_t pbuf, reg_t len, reg_t a3, reg_t a4, reg_t a5, reg_t a6) { std::vector buf(len); - ssize_t ret = read(fds.lookup(fd), &buf[0], len); + ssize_t ret = read(fds.lookup(fd), buf.data(), len); reg_t ret_errno = sysret_errno(ret); if (ret > 0) - memif->write(pbuf, ret, &buf[0]); + memif->write(pbuf, ret, buf.data()); return ret_errno; } reg_t syscall_t::sys_pread(reg_t fd, reg_t pbuf, reg_t len, reg_t off, reg_t a4, reg_t a5, reg_t a6) { std::vector buf(len); - ssize_t ret = pread(fds.lookup(fd), &buf[0], len, off); + ssize_t ret = pread(fds.lookup(fd), buf.data(), len, off); reg_t ret_errno = sysret_errno(ret); if (ret > 0) - memif->write(pbuf, ret, &buf[0]); + memif->write(pbuf, ret, buf.data()); return ret_errno; } reg_t syscall_t::sys_write(reg_t fd, reg_t pbuf, reg_t len, reg_t a3, reg_t a4, reg_t a5, reg_t a6) { std::vector buf(len); - memif->read(pbuf, len, &buf[0]); - reg_t ret = sysret_errno(write(fds.lookup(fd), &buf[0], len)); + memif->read(pbuf, len, buf.data()); + reg_t ret = sysret_errno(write(fds.lookup(fd), buf.data(), len)); return ret; } reg_t syscall_t::sys_pwrite(reg_t fd, reg_t pbuf, reg_t len, reg_t off, reg_t a4, reg_t a5, reg_t a6) { std::vector buf(len); - memif->read(pbuf, len, &buf[0]); - reg_t ret = sysret_errno(pwrite(fds.lookup(fd), &buf[0], len, off)); + memif->read(pbuf, len, buf.data()); + reg_t ret = sysret_errno(pwrite(fds.lookup(fd), buf.data(), len, off)); return ret; } @@ -291,10 +291,10 @@ reg_t syscall_t::sys_ftruncate(reg_t fd, reg_t len, reg_t a2, reg_t a3, reg_t a4 reg_t syscall_t::sys_lstat(reg_t pname, reg_t len, reg_t pbuf, reg_t a3, reg_t a4, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); + memif->read(pname, len, name.data()); struct stat buf; - reg_t ret = sysret_errno(lstat(do_chroot(&name[0]).c_str(), &buf)); + reg_t ret = sysret_errno(lstat(do_chroot(name.data()).c_str(), &buf)); if (ret != (reg_t)-1) { riscv_stat rbuf(buf, htif); @@ -309,10 +309,10 @@ reg_t syscall_t::sys_statx(reg_t fd, reg_t pname, reg_t len, reg_t flags, reg_t return -ENOSYS; #else std::vector name(len); - memif->read(pname, len, &name[0]); + memif->read(pname, len, name.data()); struct statx buf; - reg_t ret = sysret_errno(statx(fds.lookup(fd), do_chroot(&name[0]).c_str(), flags, mask, &buf)); + reg_t ret = sysret_errno(statx(fds.lookup(fd), do_chroot(name.data()).c_str(), flags, mask, &buf)); if (ret != (reg_t)-1) { riscv_statx rbuf(buf, htif); @@ -328,8 +328,8 @@ reg_t syscall_t::sys_statx(reg_t fd, reg_t pname, reg_t len, reg_t flags, reg_t reg_t syscall_t::sys_openat(reg_t dirfd, reg_t pname, reg_t len, reg_t flags, reg_t mode, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); - int fd = sysret_errno(AT_SYSCALL(openat, dirfd, &name[0], flags, mode)); + memif->read(pname, len, name.data()); + int fd = sysret_errno(AT_SYSCALL(openat, dirfd, name.data(), flags, mode)); if (fd < 0) return sysret_errno(-1); return fds.alloc(fd); @@ -338,10 +338,10 @@ reg_t syscall_t::sys_openat(reg_t dirfd, reg_t pname, reg_t len, reg_t flags, re reg_t syscall_t::sys_fstatat(reg_t dirfd, reg_t pname, reg_t len, reg_t pbuf, reg_t flags, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); + memif->read(pname, len, name.data()); struct stat buf; - reg_t ret = sysret_errno(AT_SYSCALL(fstatat, dirfd, &name[0], &buf, flags)); + reg_t ret = sysret_errno(AT_SYSCALL(fstatat, dirfd, name.data(), &buf, flags)); if (ret != (reg_t)-1) { riscv_stat rbuf(buf, htif); @@ -353,53 +353,53 @@ reg_t syscall_t::sys_fstatat(reg_t dirfd, reg_t pname, reg_t len, reg_t pbuf, re reg_t syscall_t::sys_faccessat(reg_t dirfd, reg_t pname, reg_t len, reg_t mode, reg_t a4, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); - return sysret_errno(AT_SYSCALL(faccessat, dirfd, &name[0], mode, 0)); + memif->read(pname, len, name.data()); + return sysret_errno(AT_SYSCALL(faccessat, dirfd, name.data(), mode, 0)); } reg_t syscall_t::sys_renameat(reg_t odirfd, reg_t popath, reg_t olen, reg_t ndirfd, reg_t pnpath, reg_t nlen, reg_t a6) { std::vector opath(olen), npath(nlen); - memif->read(popath, olen, &opath[0]); - memif->read(pnpath, nlen, &npath[0]); - return sysret_errno(renameat(fds.lookup(odirfd), int(odirfd) == RISCV_AT_FDCWD ? do_chroot(&opath[0]).c_str() : &opath[0], - fds.lookup(ndirfd), int(ndirfd) == RISCV_AT_FDCWD ? do_chroot(&npath[0]).c_str() : &npath[0])); + memif->read(popath, olen, opath.data()); + memif->read(pnpath, nlen, npath.data()); + return sysret_errno(renameat(fds.lookup(odirfd), int(odirfd) == RISCV_AT_FDCWD ? do_chroot(opath.data()).c_str() : opath.data(), + fds.lookup(ndirfd), int(ndirfd) == RISCV_AT_FDCWD ? do_chroot(npath.data()).c_str() : npath.data())); } reg_t syscall_t::sys_linkat(reg_t odirfd, reg_t poname, reg_t olen, reg_t ndirfd, reg_t pnname, reg_t nlen, reg_t flags) { std::vector oname(olen), nname(nlen); - memif->read(poname, olen, &oname[0]); - memif->read(pnname, nlen, &nname[0]); - return sysret_errno(linkat(fds.lookup(odirfd), int(odirfd) == RISCV_AT_FDCWD ? do_chroot(&oname[0]).c_str() : &oname[0], - fds.lookup(ndirfd), int(ndirfd) == RISCV_AT_FDCWD ? do_chroot(&nname[0]).c_str() : &nname[0], + memif->read(poname, olen, oname.data()); + memif->read(pnname, nlen, nname.data()); + return sysret_errno(linkat(fds.lookup(odirfd), int(odirfd) == RISCV_AT_FDCWD ? do_chroot(oname.data()).c_str() : oname.data(), + fds.lookup(ndirfd), int(ndirfd) == RISCV_AT_FDCWD ? do_chroot(nname.data()).c_str() : nname.data(), flags)); } reg_t syscall_t::sys_unlinkat(reg_t dirfd, reg_t pname, reg_t len, reg_t flags, reg_t a4, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); - return sysret_errno(AT_SYSCALL(unlinkat, dirfd, &name[0], flags)); + memif->read(pname, len, name.data()); + return sysret_errno(AT_SYSCALL(unlinkat, dirfd, name.data(), flags)); } reg_t syscall_t::sys_mkdirat(reg_t dirfd, reg_t pname, reg_t len, reg_t mode, reg_t a4, reg_t a5, reg_t a6) { std::vector name(len); - memif->read(pname, len, &name[0]); - return sysret_errno(AT_SYSCALL(mkdirat, dirfd, &name[0], mode)); + memif->read(pname, len, name.data()); + return sysret_errno(AT_SYSCALL(mkdirat, dirfd, name.data(), mode)); } reg_t syscall_t::sys_getcwd(reg_t pbuf, reg_t size, reg_t a2, reg_t a3, reg_t a4, reg_t a5, reg_t a6) { std::vector buf(size); - char* ret = getcwd(&buf[0], size); + char* ret = getcwd(buf.data(), size); if (ret == NULL) return sysret_errno(-1); - std::string tmp = undo_chroot(&buf[0]); + std::string tmp = undo_chroot(buf.data()); if (size <= tmp.size()) return -ENOMEM; - memif->write(pbuf, tmp.size() + 1, &tmp[0]); + memif->write(pbuf, tmp.size() + 1, tmp.data()); return tmp.size() + 1; } @@ -419,14 +419,14 @@ reg_t syscall_t::sys_getmainvars(reg_t pbuf, reg_t limit, reg_t a2, reg_t a3, re } std::vector bytes(sz); - memcpy(&bytes[0], &words[0], sizeof(words[0]) * words.size()); + memcpy(bytes.data(), words.data(), sizeof(words[0]) * words.size()); for (size_t i = 0; i < args.size(); i++) strcpy(&bytes[htif->from_target(words[i+1]) - pbuf], args[i].c_str()); if (bytes.size() > limit) return -ENOMEM; - memif->write(pbuf, bytes.size(), &bytes[0]); + memif->write(pbuf, bytes.size(), bytes.data()); return 0; } -- cgit v1.1