In `hpt_status` -> `hpt_set_info`, `nOutBufferSize` and `nInBufferSize` are checked at the same time, but not individually: if (piop->nInBufferSize+piop->nOutBufferSize > PAGE_SIZE) { KdPrintE(("User buffer too large\n")); return -EINVAL; } Before performing a kernel allocation: ke_area = malloc(piop->nInBufferSize+piop->nOutBufferSize, M_DEVBUF, M_NOWAIT); However, the sizes are later used individually for some copies: if (piop->nInBufferSize) copyin((void*)(ULONG_PTR)piop->lpInBuffer, ke_area, piop->nInBufferSize); ... if (piop->nOutBufferSize) copyout(ke_area + piop->nInBufferSize, (void*)(ULONG_PTR)piop->lpOutBuffer, piop->nOutBufferSize); It might be possible for `nInBufferSize`, or `outBufferSize`, or both, to be large enough for `piop->nInBufferSize+piop->nOutBufferSize` to overflow and be less than `PAGE_SIZE`. In this situation the copy calls would result in a heap overflow.
These sizes are defined as `DWORD`, a `typedef` for `unsigned int`, rather than a 64bit type like `size_t`, so getting the sum of both sizes to overflow doesn't seem possible.
Not sure what I was talking about earlier, but I've decompiled the module to produce more easily analysable code: if ( (unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7)) <= 0x1000 ) { ptr = malloc((unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7))); if ( ptr ) { if ( *((_DWORD *)&hptproc_buffer + 4) ) copyin(*((_QWORD *)&hptproc_buffer + 1), ptr, *((_DWORD *)&hptproc_buffer + 4)); Now it's clear that the comparison on the sum of the two sizes is treated as `unsigned int`. So, theoretically: `nInBufferSize` of `0xffffffff`, `nOutBufferSize` of `1`, The sum of these two sizes will overflow to `0`, so the check will pass: if ( (unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7)) <= 0x1000 ) The size passed to `malloc` will also be `0`. However, `copyin` will copy from `nInBufferSize` of `0xffffffff`. To exploit this, the size of the allocation could be controlled to manipulate the heap layout to reliably target the overflow.
PoC code from the above explanation, which results in panic: https://gist.github.com/CTurt/696a34664bc8d4f4e905
Supplying the `HPT_IOCTL_GET_EVENT` command will ensure that `Kernel_DeviceIoControl` function instantly returns, resulting in `hpt_set_info` returning straight after doing the `malloc`, `copyin`, and `free`: case HPT_IOCTL_GET_EVENT: { PHPT_EVENT pInfo; if (nInBufferSize!=0) return -1; I've also refined the `size` related parameters needed to fully control the heap overflow: params.dwIoControlCode = HPT_IOCTL_GET_EVENT; params.lpInBuffer = mapping; params.nInBufferSize = bufferSize + overflowSize; params.lpOutBuffer = NULL; params.nOutBufferSize = -overflowSize; params.lpBytesReturned = &bytesReturned; printf(" [+] nInBufferSize (size copied in): %08x\n", params.nInBufferSize); printf(" [+] nOutBufferSize: %08x\n", params.nOutBufferSize); printf(" [+] Sum (allocation size): %08x\n", params.nInBufferSize + params.nOutBufferSize); printf(" [+] Will be accepted: %d\n", (params.nInBufferSize + params.nOutBufferSize) <= PAGE_SIZE); You can get very manageable sizes from this, for example, allocated `bufferSize` of `0x500`, and copy size of `0x1000`. I'll try to get around to writing a full exploit for this soon, but won't be very useful since the vulnerability is only triggerable as `root` anyway.
I'd also like to add that the result of `copyin` isn't checked here, which can lead to use of initialised heap buffer (it is not allocated with `M_ZERO`).
Any movement on this?
Created attachment 169074 [details] Fix heap overflow and check result of copyin
(In reply to CTurt from comment #7) Is the last bit of the diff just whitespace?
Yes. This file has some indentation inconsistencies: most parts use tabs, but there are occasional lines which use spaces for indentation; my editor accidentally replaced the spaces with tabs for one of these lines.
A commit references this bug: Author: sbruno Date: Mon Apr 18 23:26:11 UTC 2016 New revision: 298231 URL: https://svnweb.freebsd.org/changeset/base/298231 Log: hptmv(4) Fix potential buffer overflow in hpt_set_info. While here, adjust some whitespace and yeild some useful debug info. This is untested on this hardware, testing requests to -scsi went unanswered. PR: 206585 Submitted by: cturt@hardenedbsd.org MFC after: 2 weeks Changes: head/sys/dev/hptmv/hptproc.c
Patch was committed, closing as fixed