From edd51d7f43ab5e511c72bac83f6286ecc2403251 Mon Sep 17 00:00:00 2001 From: Andrew Lewycky Date: Wed, 4 Sep 2024 16:13:09 +0000 Subject: [PATCH 1/5] Add a test for TENSTORRENT_IOCTL_GET_DRIVER_INFO --- test/Makefile | 3 ++- test/get_driver_info.cpp | 37 +++++++++++++++++++++++++++++++++++++ test/main.cpp | 2 ++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 test/get_driver_info.cpp diff --git a/test/Makefile b/test/Makefile index 192a273..86047df 100644 --- a/test/Makefile +++ b/test/Makefile @@ -5,7 +5,8 @@ all:: PROG := ttkmd_test -TEST_SOURCES := get_device_info.cpp query_mappings.cpp dma_buf.cpp pin_pages.cpp config_space.cpp lock.cpp hwmon.cpp map_peer_bar.cpp +TEST_SOURCES := get_driver_info.cpp get_device_info.cpp query_mappings.cpp \ + dma_buf.cpp pin_pages.cpp config_space.cpp lock.cpp hwmon.cpp map_peer_bar.cpp CORE_SOURCES := enumeration.cpp util.cpp devfd.cpp main.cpp test_failure.cpp SOURCES := $(CORE_SOURCES) $(TEST_SOURCES) diff --git a/test/get_driver_info.cpp b/test/get_driver_info.cpp new file mode 100644 index 0000000..b5d0eb6 --- /dev/null +++ b/test/get_driver_info.cpp @@ -0,0 +1,37 @@ +// SPDX-FileCopyrightText: © 2023 Tenstorrent Inc. +// SPDX-License-Identifier: GPL-2.0-only + +#include + +#include + +#include "ioctl.h" + +#include "util.h" +#include "test_failure.h" +#include "enumeration.h" +#include "devfd.h" + +void TestGetDriverInfo(const EnumeratedDevice &dev) +{ + DevFd dev_fd(dev.path); + + tenstorrent_get_driver_info get_driver_info{}; + get_driver_info.in.output_size_bytes = sizeof(get_driver_info.out); + + if (ioctl(dev_fd.get(), TENSTORRENT_IOCTL_GET_DRIVER_INFO, &get_driver_info) != 0) + THROW_TEST_FAILURE("TENSTORRENT_IOCTL_GET_DRIVER_INFO failed on " + dev.path); + + std::size_t min_get_driver_info_out + = offsetof(tenstorrent_get_driver_info_out, driver_version) + + sizeof(get_driver_info.out.driver_version); + + if (get_driver_info.out.output_size_bytes < min_get_driver_info_out) + THROW_TEST_FAILURE("GET_DRIVER_INFO output is too small."); + + if (get_driver_info.out.output_size_bytes > sizeof(get_driver_info.out)) + THROW_TEST_FAILURE("GET_DRIVER_INFO output is too large. (Test may be out of date.)"); + + if (get_driver_info.out.driver_version != TENSTORRENT_DRIVER_VERSION) + THROW_TEST_FAILURE("GET_DRIVER_INFO reports an unexpected driver version."); +} diff --git a/test/main.cpp b/test/main.cpp index 341dfc7..5ca19c4 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -8,6 +8,7 @@ #include "enumeration.h" #include "test_failure.h" +void TestGetDriverInfo(const EnumeratedDevice &dev); void TestGetDeviceInfo(const EnumeratedDevice &dev); void TestConfigSpace(const EnumeratedDevice &dev, bool check_aer); void TestQueryMappings(const EnumeratedDevice &dev); @@ -31,6 +32,7 @@ int main(int argc, char *argv[]) { std::cout << "Testing " << d.path << " @ " << d.location.format() << '\n'; + TestGetDriverInfo(d); TestGetDeviceInfo(d); TestConfigSpace(d, check_aer); TestQueryMappings(d); From dfa17c0ca7d2481c554fb9a125f653433bb8207f Mon Sep 17 00:00:00 2001 From: Andrew Lewycky Date: Wed, 4 Sep 2024 18:30:45 +0000 Subject: [PATCH 2/5] Relocate struct Freer to util.h --- test/pin_pages.cpp | 6 ------ test/util.h | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/pin_pages.cpp b/test/pin_pages.cpp index 7739ad5..3150834 100644 --- a/test/pin_pages.cpp +++ b/test/pin_pages.cpp @@ -37,12 +37,6 @@ namespace { -struct Freer -{ - template - void operator() (T* p) const { std::free(p); } -}; - struct Unmapper { std::size_t page_count; diff --git a/test/util.h b/test/util.h index 774f513..fbbdc80 100644 --- a/test/util.h +++ b/test/util.h @@ -28,6 +28,12 @@ inline bool operator != (const PciBusDeviceFunction &l, const PciBusDeviceFuncti return !(l == r); } +struct Freer +{ + template + void operator() (T* p) const { std::free(p); } +}; + template static inline void zero(T* p) { From 03c650f19e91cc0fbf7ee700265f4ba1aab32b77 Mon Sep 17 00:00:00 2001 From: Andrew Lewycky Date: Wed, 4 Sep 2024 19:27:37 +0000 Subject: [PATCH 3/5] Add ioctl user-mode access overrun tests With in.output_size_bytes = 0, verify that there are no accesses past the input structure. For ioctls that don't have in.output_size_bytes, verify there are no accesses past the entire input/output structure. --- test/Makefile | 3 +- test/ioctl_overrun.cpp | 212 +++++++++++++++++++++++++++++++++++++++++ test/main.cpp | 2 + test/util.h | 7 ++ 4 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 test/ioctl_overrun.cpp diff --git a/test/Makefile b/test/Makefile index 86047df..3ab8775 100644 --- a/test/Makefile +++ b/test/Makefile @@ -6,7 +6,8 @@ all:: PROG := ttkmd_test TEST_SOURCES := get_driver_info.cpp get_device_info.cpp query_mappings.cpp \ - dma_buf.cpp pin_pages.cpp config_space.cpp lock.cpp hwmon.cpp map_peer_bar.cpp + dma_buf.cpp pin_pages.cpp config_space.cpp lock.cpp hwmon.cpp map_peer_bar.cpp \ + ioctl_overrun.cpp CORE_SOURCES := enumeration.cpp util.cpp devfd.cpp main.cpp test_failure.cpp SOURCES := $(CORE_SOURCES) $(TEST_SOURCES) diff --git a/test/ioctl_overrun.cpp b/test/ioctl_overrun.cpp new file mode 100644 index 0000000..e839129 --- /dev/null +++ b/test/ioctl_overrun.cpp @@ -0,0 +1,212 @@ +// Try to catch ioctls that read or write the wrong amount of data. +// +// When an ioctl input has output_size_bytes, we align the input to the end of the page +// and set output_size_bytes = 0. This should result in no output being written and no error. +// This catches read and write overruns. +// +// When an ioctl input doesn't have output_size_bytes, we align the entire structure to the +// end of the page. This catches write overruns. +// If hardware had support for PROT_WRITE without PROT_READ we could also check for read overruns. + +#include +#include + +#include +#include +#include +#include + +#include +#include + +#include "devfd.h" +#include "enumeration.h" +#include "ioctl.h" +#include "test_failure.h" +#include "util.h" + +namespace +{ + +// Allocate data aligned to the end of a page, guaranteeing that the next page is unmapped. +template +class EndOfPage +{ +public: + EndOfPage(const T& init = {}); + ~EndOfPage(); + + EndOfPage(const EndOfPage&) = delete; + void operator = (const EndOfPage&) = delete; + + T *get(); + +private: + void *mapping = nullptr; + T *value = nullptr; + + static std::size_t mapping_size(); +}; + +template +std::size_t EndOfPage::mapping_size() +{ + return round_up(sizeof(T), page_size()) + page_size(); +} + +template +EndOfPage::EndOfPage(const T& init) +{ + auto size = mapping_size(); + + mapping = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + if (mapping == MAP_FAILED) + throw_system_error("end-of-page mapping allocation failed"); + + void *final_page = reinterpret_cast(reinterpret_cast(mapping) + size - page_size()); + + if (mprotect(final_page, page_size(), PROT_NONE) != 0) + throw_system_error("failed to disable access to overrun detection page"); + + void *p = reinterpret_cast(reinterpret_cast(final_page) - sizeof(T)); + value = new (p) T(init); +} + +template +EndOfPage::~EndOfPage() +{ + value->~T(); + munmap(mapping, mapping_size()); +} + +template +T *EndOfPage::get() +{ + return value; +} + +// The assumption is that the ioctl_data is aligned to the end of the page and no EFAULT should occur. +#define CHECK_IOCTL_OVERRUN(fd, ioctl_name, ioctl_data) CheckIoctlOverrun(fd, ioctl_name, #ioctl_name, ioctl_data) +#define CHECK_IOCTL_OVERRUN_ERROR(fd, ioctl_name, ioctl_data, expected_error) CheckIoctlOverrun(fd, ioctl_name, #ioctl_name, ioctl_data, expected_error) + +template +void CheckIoctlOverrun(int fd, unsigned long ioctl_code, const char *ioctl_name, const IoctlData& ioctl_data, int expected_error = 0) +{ + EndOfPage aligned_ioctl_data(ioctl_data); + + int result = ioctl(fd, ioctl_code, aligned_ioctl_data.get()); + + if (result != 0) + { + if (errno == EFAULT) + THROW_TEST_FAILURE(std::string(ioctl_name) + " failed overrun check."); + else if (errno != expected_error) + THROW_TEST_FAILURE(std::string(ioctl_name) + " overrun check failed other than EFAULT."); + } +} + +void TestGetDeviceInfoOverrun(int fd) +{ + tenstorrent_get_device_info_in in{}; + in.output_size_bytes = 0; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_GET_DEVICE_INFO, in); +} + +void TestQueryMappingsOverrun(int fd) +{ + tenstorrent_query_mappings_in in{}; + in.output_mapping_count = 0; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_QUERY_MAPPINGS, in); +} + +void TestAllocateDmaBufOverrun(int fd) +{ + tenstorrent_allocate_dma_buf alloc_buf{}; + + alloc_buf.in.requested_size = page_size(); + alloc_buf.in.buf_index = 0; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_ALLOCATE_DMA_BUF, alloc_buf); +} + +void TestFreeDmaBufOverrun(int fd) +{ + tenstorrent_free_dma_buf free_buf{}; + + CHECK_IOCTL_OVERRUN_ERROR(fd, TENSTORRENT_IOCTL_FREE_DMA_BUF, free_buf, EINVAL); +} + +void TestGetDriverInfoOverrun(int fd) +{ + tenstorrent_get_driver_info_in in{}; + in.output_size_bytes = 0; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_GET_DRIVER_INFO, in); +} + +void TestResetDeviceOverrun(int fd) +{ + tenstorrent_reset_device_in in{}; + in.output_size_bytes = 0; + in.flags = TENSTORRENT_RESET_DEVICE_RESTORE_STATE; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_RESET_DEVICE, in); +} + +void TestPinPagesOverrun(int fd) +{ + std::unique_ptr page(std::aligned_alloc(page_size(), page_size())); + + tenstorrent_pin_pages_in in{}; + in.output_size_bytes = 0; + in.virtual_address = reinterpret_cast(page.get()); + in.size = page_size(); + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_PIN_PAGES, in); +} + +void TestLockCtlOverrun(int fd) +{ + tenstorrent_lock_ctl_in in{}; + in.output_size_bytes = 0; + in.flags = TENSTORRENT_LOCK_CTL_TEST; + in.index = 0; + + CHECK_IOCTL_OVERRUN(fd, TENSTORRENT_IOCTL_LOCK_CTL, in); +} + +void TestMapPeerBarOverrun(int fd) +{ + // TENSTORRENT_IOCTL_MAP_PEER_BAR requires 2 devices and doesn't have output_size_bytes + // so we can only test that it rejects the input without EFAULT. + + tenstorrent_map_peer_bar_in in{}; + + in.peer_fd = fd; + in.peer_bar_index = 0; + in.peer_bar_offset = 0; + in.peer_bar_length = page_size(); + in.flags = 0; + + CHECK_IOCTL_OVERRUN_ERROR(fd, TENSTORRENT_IOCTL_MAP_PEER_BAR, in, EINVAL); +} + +} + +void TestIoctlOverrun(const EnumeratedDevice &dev) +{ + DevFd dev_fd(dev.path); + + TestGetDeviceInfoOverrun(dev_fd.get()); + // TENSTORRENT_IOCTL_GET_HARVESTING simply fails. + TestQueryMappingsOverrun(dev_fd.get()); + TestAllocateDmaBufOverrun(dev_fd.get()); + TestFreeDmaBufOverrun(dev_fd.get()); + TestGetDriverInfoOverrun(dev_fd.get()); + TestResetDeviceOverrun(dev_fd.get()); + TestPinPagesOverrun(dev_fd.get()); + TestLockCtlOverrun(dev_fd.get()); + TestMapPeerBarOverrun(dev_fd.get()); +} diff --git a/test/main.cpp b/test/main.cpp index 5ca19c4..d24d229 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -16,6 +16,7 @@ void TestDmaBuf(const EnumeratedDevice &dev); void TestPinPages(const EnumeratedDevice &dev); void TestLock(const EnumeratedDevice &dev); void TestHwmon(const EnumeratedDevice &dev); +void TestIoctlOverrun(const EnumeratedDevice &dev); void TestMapPeerBar(const EnumeratedDevice &dev1, const EnumeratedDevice &dev2); int main(int argc, char *argv[]) @@ -40,6 +41,7 @@ int main(int argc, char *argv[]) TestPinPages(d); TestLock(d); TestHwmon(d); + TestIoctlOverrun(d); at_least_one_device = true; } diff --git a/test/util.h b/test/util.h index fbbdc80..3e80583 100644 --- a/test/util.h +++ b/test/util.h @@ -61,3 +61,10 @@ std::string sysfs_dir_for_bdf(PciBusDeviceFunction bdf); unsigned page_size(); int make_anonymous_temp(); + +template +T round_up(T x, U alignment) +{ + auto remainder = x % alignment; + return x + ((remainder == 0) ? 0 : alignment) - remainder; +} From e5b6bd6e1691b03d8db298818845c79771fedcb4 Mon Sep 17 00:00:00 2001 From: Andrew Lewycky Date: Thu, 5 Sep 2024 17:27:21 +0000 Subject: [PATCH 4/5] Add ioctl output zeroing tests ioctls with in.output_size_bytes are expected to clear the output before writing. (This is intended for new-user/old-kernel compatibility if the outputs are extended.) --- test/Makefile | 2 +- test/ioctl_zeroing.cpp | 107 +++++++++++++++++++++++++++++++++++++++++ test/main.cpp | 2 + 3 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 test/ioctl_zeroing.cpp diff --git a/test/Makefile b/test/Makefile index 3ab8775..cef7e80 100644 --- a/test/Makefile +++ b/test/Makefile @@ -7,7 +7,7 @@ PROG := ttkmd_test TEST_SOURCES := get_driver_info.cpp get_device_info.cpp query_mappings.cpp \ dma_buf.cpp pin_pages.cpp config_space.cpp lock.cpp hwmon.cpp map_peer_bar.cpp \ - ioctl_overrun.cpp + ioctl_overrun.cpp ioctl_zeroing.cpp CORE_SOURCES := enumeration.cpp util.cpp devfd.cpp main.cpp test_failure.cpp SOURCES := $(CORE_SOURCES) $(TEST_SOURCES) diff --git a/test/ioctl_zeroing.cpp b/test/ioctl_zeroing.cpp new file mode 100644 index 0000000..164225c --- /dev/null +++ b/test/ioctl_zeroing.cpp @@ -0,0 +1,107 @@ +// Some ioctls have an output_size_bytes input value. When the actual output +// data is smaller than output_size_bytes, the remainder must be zero-filled. + +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +#include "aligned_allocator.h" +#include "devfd.h" +#include "enumeration.h" +#include "ioctl.h" +#include "test_failure.h" +#include "util.h" + +namespace +{ + +#define CHECK_IOCTL_ZEROING(fd, ioctl_name, ioctl_data) CheckIoctlZeroing(fd, ioctl_name, #ioctl_name, ioctl_data) + +template +void CheckIoctlZeroing(int fd, unsigned long ioctl_code, const char *ioctl_name, const IoctlData &ioctl_data) +{ + std::vector> buf; + buf.resize(offsetof(IoctlData, out) + page_size(), 0xFF); + + IoctlData *ioctl_param = new (buf.data()) IoctlData(ioctl_data); + + if (ioctl(fd, ioctl_code, ioctl_param) != 0) + THROW_TEST_FAILURE(std::string(ioctl_name) + " ioctl errored in zeroing test."); + + if (std::find_if(buf.begin()+sizeof(IoctlData), buf.end(), + [] (unsigned char c) { return c != 0; }) != buf.end()) + THROW_TEST_FAILURE(std::string(ioctl_name) + " did not zero the entire output range."); +} + +void TestGetDeviceInfoZeroing(int fd) +{ + tenstorrent_get_device_info get_device_info{}; + get_device_info.in.output_size_bytes = page_size(); + + CHECK_IOCTL_ZEROING(fd, TENSTORRENT_IOCTL_GET_DEVICE_INFO, get_device_info); +} + +void TestGetDriverInfoZeroing(int fd) +{ + tenstorrent_get_driver_info get_driver_info{}; + get_driver_info.in.output_size_bytes = page_size(); + + CHECK_IOCTL_ZEROING(fd, TENSTORRENT_IOCTL_GET_DRIVER_INFO, get_driver_info); +} + +void TestResetDeviceZeroing(int fd) +{ + tenstorrent_reset_device reset_device{}; + reset_device.in.output_size_bytes = page_size(); + reset_device.in.flags = TENSTORRENT_RESET_DEVICE_RESTORE_STATE; + + CHECK_IOCTL_ZEROING(fd, TENSTORRENT_IOCTL_RESET_DEVICE, reset_device); +} + +void TestPinPagesZeroing(int fd) +{ + std::unique_ptr page(std::aligned_alloc(page_size(), page_size())); + + tenstorrent_pin_pages pin_pages{}; + pin_pages.in.output_size_bytes = page_size(); + pin_pages.in.virtual_address = reinterpret_cast(page.get()); + pin_pages.in.size = page_size(); + + CHECK_IOCTL_ZEROING(fd, TENSTORRENT_IOCTL_PIN_PAGES, pin_pages); +} + +void TestLockCtlZeroing(int fd) +{ + tenstorrent_lock_ctl lock_ctl{}; + lock_ctl.in.output_size_bytes = page_size(); + lock_ctl.in.flags = TENSTORRENT_LOCK_CTL_TEST; + lock_ctl.in.index = 0; + + CHECK_IOCTL_ZEROING(fd, TENSTORRENT_IOCTL_LOCK_CTL, lock_ctl); +} + +} + +void TestIoctlZeroing(const EnumeratedDevice &dev) +{ + DevFd dev_fd(dev.path); + + TestGetDeviceInfoZeroing(dev_fd.get()); + // TENSTORRENT_IOCTL_GET_HARVESTING simply fails. + // TENSTORRENT_IOCTL_QUERY_MAPPINGS is complicated, has its own test. + // TENSTORRENT_IOCTL_ALLOCATE_DMA_BUF does not zero. + // TENSTORRENT_IOCTL_FREE_DMA_BUF does not zero. + TestGetDriverInfoZeroing(dev_fd.get()); + TestResetDeviceZeroing(dev_fd.get()); + TestPinPagesZeroing(dev_fd.get()); + TestLockCtlZeroing(dev_fd.get()); + // TENSTORRENT_IOCTL_MAP_PEER_BAR does not zero. +} diff --git a/test/main.cpp b/test/main.cpp index d24d229..369d549 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -17,6 +17,7 @@ void TestPinPages(const EnumeratedDevice &dev); void TestLock(const EnumeratedDevice &dev); void TestHwmon(const EnumeratedDevice &dev); void TestIoctlOverrun(const EnumeratedDevice &dev); +void TestIoctlZeroing(const EnumeratedDevice &dev); void TestMapPeerBar(const EnumeratedDevice &dev1, const EnumeratedDevice &dev2); int main(int argc, char *argv[]) @@ -42,6 +43,7 @@ int main(int argc, char *argv[]) TestLock(d); TestHwmon(d); TestIoctlOverrun(d); + TestIoctlZeroing(d); at_least_one_device = true; } From ce039538bad42a31924e35a5009ac607185b18e7 Mon Sep 17 00:00:00 2001 From: Andrew Lewycky Date: Wed, 4 Sep 2024 19:38:25 +0000 Subject: [PATCH 5/5] Fix ioctl input & output sizes Fix get_device_info and get_driver_info input size. The output structure was accidentally used as the input structure. As it happens, the output structure starts with the only field in the input structure (output_size_bytes) so there's no effect under as long as a complete input & output are passed. Fix get_device_info, get_driver_info and reset_device output size. The output should have been capped to in.output_size_bytes but the output structure was always written in full. --- chardev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/chardev.c b/chardev.c index 07f89f9..f8c36a8 100644 --- a/chardev.c +++ b/chardev.c @@ -110,7 +110,7 @@ static long ioctl_get_device_info(struct chardev_private *priv, const struct pci_dev *pdev = priv->device->pdev; u32 bytes_to_copy; - struct tenstorrent_get_device_info_out in; + struct tenstorrent_get_device_info_in in; struct tenstorrent_get_device_info_out out; memset(&in, 0, sizeof(in)); memset(&out, 0, sizeof(out)); @@ -132,7 +132,7 @@ static long ioctl_get_device_info(struct chardev_private *priv, bytes_to_copy = min(in.output_size_bytes, (u32)sizeof(out)); - if (copy_to_user(&arg->out, &out, sizeof(out)) != 0) + if (copy_to_user(&arg->out, &out, bytes_to_copy) != 0) return -EFAULT; return 0; @@ -143,7 +143,7 @@ static long ioctl_get_driver_info(struct chardev_private *priv, { u32 bytes_to_copy; - struct tenstorrent_get_driver_info_out in; + struct tenstorrent_get_driver_info_in in; struct tenstorrent_get_driver_info_out out; memset(&in, 0, sizeof(in)); memset(&out, 0, sizeof(out)); @@ -159,7 +159,7 @@ static long ioctl_get_driver_info(struct chardev_private *priv, bytes_to_copy = min(in.output_size_bytes, (u32)sizeof(out)); - if (copy_to_user(&arg->out, &out, sizeof(out)) != 0) + if (copy_to_user(&arg->out, &out, bytes_to_copy) != 0) return -EFAULT; return 0; @@ -200,7 +200,7 @@ static long ioctl_reset_device(struct chardev_private *priv, bytes_to_copy = min(in.output_size_bytes, (u32)sizeof(out)); - if (copy_to_user(&arg->out, &out, sizeof(out)) != 0) + if (copy_to_user(&arg->out, &out, bytes_to_copy) != 0) return -EFAULT; return 0;