From 4903487f21fc277c60c6daf223f3070d5ad4fe6c Mon Sep 17 00:00:00 2001 From: Sebastian Sumpf Date: Thu, 18 May 2023 21:22:03 +0200 Subject: [PATCH] vbox6: fix "Error: slab block [xxx] is corrupt" In 'SUPR3InitEx' (SUPLib.cpp) a 'SUPQUERYFUNCS' structure is allocated with ! (PSUPQUERYFUNCS)RTMemAllocZ(SUP_IOCTL_QUERY_FUNCS_SIZE(CookieReq.u.Out.cFunctions)); where 'CookieReq.u.Out.cFunctions' is 0. To determine the size of the allocation ! #define SUP_IOCTL_QUERY_FUNCS_SIZE(cFuncs) \ ! RT_UOFFSETOF_DYN(SUPQUERYFUNCS, u.Out.aFunctions[(cFuncs)]) is used with cFuncs = 0 (SUPDrvIOC.h) leading to an allocation up to the arrow below ! typedef struct SUPQUERYFUNCS ! { ! /** The header. */ ! SUPREQHDR Hdr; ! union ! { ! struct ! { ! /** Number of functions returned. */ ! uint32_t cFunctions; ! /** Array of functions. */ ==> end of allocation ! SUPFUNC aFunctions[1]; ! } Out; ! } u; ==> sizeof(SUPQUERYFUNCS) ! } SUPQUERYFUNCS, *PSUPQUERYFUNCS; In sup.cc (Genode) 'ioctl(SUPQUERYFUNCS &request)' will lead to 'with_out_ioctl' ! auto &out = request.u.Out; where auto is 'SUPQUERYFUNCS' and finally ! out = { }; will zero out 'SUPQUERYFUNCS' up to the second arrow above. Because 'RTMemAllocZ' will call 'calloc' to allocate the memory 'out = { };' will corrupt the slab block after the allocation. Therefore, it is reasonable to allocate at least 'sizeof(SUPQUERYFUNCS)'. Note there might be other 'ioctl' cases like this. A better way might be to use 'SUPQUERYFUNCS.Hdr.cbOut' to determine the 'out' size. fixes #4675 --- repos/ports/ports/virtualbox6.hash | 2 +- repos/ports/src/virtualbox6/patches/series | 1 + .../patches/sup_ioctl_query_func_size.patch | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 repos/ports/src/virtualbox6/patches/sup_ioctl_query_func_size.patch diff --git a/repos/ports/ports/virtualbox6.hash b/repos/ports/ports/virtualbox6.hash index 26e9799f31..e0cb5a5e29 100644 --- a/repos/ports/ports/virtualbox6.hash +++ b/repos/ports/ports/virtualbox6.hash @@ -1 +1 @@ -f5ab8d8b5f20dc67de05dc2cb7abd2b9cc1c8b6c +f5ebf8e03d4b29d3c186212bdc2d324bb31c5274 diff --git a/repos/ports/src/virtualbox6/patches/series b/repos/ports/src/virtualbox6/patches/series index 91fef3228f..2cb4d2ca60 100644 --- a/repos/ports/src/virtualbox6/patches/series +++ b/repos/ports/src/virtualbox6/patches/series @@ -11,3 +11,4 @@ svga.patch audio.patch gcc-12.patch pgmphys.patch +sup_ioctl_query_func_size.patch diff --git a/repos/ports/src/virtualbox6/patches/sup_ioctl_query_func_size.patch b/repos/ports/src/virtualbox6/patches/sup_ioctl_query_func_size.patch new file mode 100644 index 0000000000..64ac456a90 --- /dev/null +++ b/repos/ports/src/virtualbox6/patches/sup_ioctl_query_func_size.patch @@ -0,0 +1,17 @@ +index fa26845..c7377e8 100644 +--- a/src/virtualbox6/src/VBox/HostDrivers/Support/SUPDrvIOC.h ++++ b/src/virtualbox6/src/VBox/HostDrivers/Support/SUPDrvIOC.h +@@ -267,7 +267,12 @@ typedef struct SUPCOOKIE + * @{ + */ + #define SUP_IOCTL_QUERY_FUNCS(cFuncs) SUP_CTL_CODE_BIG(2) +-#define SUP_IOCTL_QUERY_FUNCS_SIZE(cFuncs) RT_UOFFSETOF_DYN(SUPQUERYFUNCS, u.Out.aFunctions[(cFuncs)]) ++/* ++ * Ensure to return offset of at least 'aFunctions[1]' because the array in ++ * SUPQUERYFUNC has one element. Anyone using 'sizeof(SUPQUERYFUNCS)' (e.g., ++ * constructor) will still access aFunctions[0] in case 'cFuncs' is 0. ++ */ ++#define SUP_IOCTL_QUERY_FUNCS_SIZE(cFuncs) RT_UOFFSETOF_DYN(SUPQUERYFUNCS, u.Out.aFunctions[(cFuncs ? cFuncs : 1)]) + #define SUP_IOCTL_QUERY_FUNCS_SIZE_IN sizeof(SUPREQHDR) + #define SUP_IOCTL_QUERY_FUNCS_SIZE_OUT(cFuncs) SUP_IOCTL_QUERY_FUNCS_SIZE(cFuncs) +