Bug #13768
closedmpapi is too clever with its ioctls tripping SSP
100%
Description
Using mpathadm, it would segfault with the following stack:
> ::status debugging core file of mpathadm (32-bit) from bsrscale02 file: /usr/sbin/mpathadm initial argv: mpathadm list LU threading model: native threads status: process panicked upanic message: *** stack smashing detected > ::stack libc.so.1`syscall+0x13(feef94c4, 1c, c2645bb3, 0, 803e4e8, fed90000) 0xfee7d65f(6) libmpscsi_vhci.so.1`__stack_chk_fail_local+0x17(0, 0, 0, 803e538, 0, 5) 0xfed86fb1(4, 1, 0, ee, 803e980, fedb5a88) libMPAPI.so.1`MP_GetMPLogicalUnitProperties+0xa6(4, 1) listLogicalUnit+0x9b4(0, 8047d74, 80419a4, 80413b4) listFunc+0x66(0, 8047d74, 2, 80419a4, 0, 1) cmdParse+0xa62(3, 8047d68, 8047cd2, 806b520, 806b4c0, 806b480) main+0x97(8047cfc, fef695c8, 8047d38, 805282c) _start_crt+0x9b(3, 8047d68, fefd0d9c, 0, 0, 0) _start+0x1a(3, 8047e4c, 8047e55, 8047e5a, 0, 8047e5d)
As best as I can tell, the multipath ioctls pass in mp_iocdata_t
which itself contains the userland addresses (and sizes) of the actual buffers to read/write data for the ioctl.
Several of these ioctls end with a field of type caddr_t
which is the problem -- 64-bit versions of these structs will be 4 bytes larger than the 32-bit versions (defeating the care noted in usr/src/uts/common/sys/scsi/adapters/mpapi_impl.h
in the definition of the structs).
As an example, with the above stack, we pass the address of mp_logical_unit_prop_t
in, and expect the kernel to copy out results into luinfo
:
...
mp_iocdata_t mp_ioctl;
mp_logical_unit_prop_t luInfo;
...
mp_ioctl.mp_cmd = MP_GET_LU_PROP;
mp_ioctl.mp_ibuf = (caddr_t)&oid.objectSequenceNumber;
mp_ioctl.mp_ilen = sizeof (oid.objectSequenceNumber);
mp_ioctl.mp_obuf = (caddr_t)&luInfo;
mp_ioctl.mp_olen = sizeof (mp_logical_unit_prop_t);
mp_ioctl.mp_xfer = MP_XFER_READ;
ioctlStatus = ioctl(g_scsi_vhci_fd, MP_CMD, &mp_ioctl);
The kernel does:
if (ddi_copyout((void *)mplup_prop, mpioc->mp_obuf,
sizeof (mp_logical_unit_prop_t), mode) != 0) {
return (EFAULT);
}
Thus causing an extra 4 bytes of data to be written beyond the end of luinfo
, triggering the stack smashing protection. Amazingly, the code seems to be semi-aware of this. In vhci_mpapi_validate
, it validates that the mp_olen
value passed in is sufficiently large for the results. For 32-bit callers, it subtracts 4 from the size of struct when it compares the mp_olen
value:
case MP_GET_LU_PROP:
{
olen = sizeof (mp_logical_unit_prop_t);
/* Adjust olen to account for the caddr_t in 32-bit mode */
if (mode32 == 1) {
olen -= 4;
}
Files
Updated by Carsten Grzemba about 2 years ago
I see this problem on Omnios r151038 and want to do some investigation.
For that how can I activate the log output in MP_GetMPLogicalUnitProperties.c
log(LOG_INFO, "MP_GetMPLogicalUnitProperties()", " - enter");
log(LOG_INFO, "MP_GetMPLogicalUnitProperties()",
This is still in userland before the ioctl().
Updated by Carsten Grzemba about 2 years ago
- File issue-13768.patch issue-13768.patch added
One possible solution would be to move the problematic data from stack to heap, like attached patch.
With this changes works mpathadm for me.
Updated by Jason King about 2 years ago
That just changes stack corruption to heap corruption (I suspect if you ran that with UMEM_DEBUG=default, it'd also crash).
I have a fix I'll try to put up for review soon. Given that the field isn't used by user land or the kernel currently (and is private), there's a bit of a simpler solution.
Updated by Jason King almost 2 years ago
We've been using this fix at Racktop for a few months, and verified that with this patch, mpathadm no longer triggers SSP.
Updated by Jason King over 1 year ago
Additionally, an omnios user that was hitting this issue tried a hot fix containing this change and verified that it also resolved the issue for them: https://illumos.topicbox.com/groups/omnios-discuss/T3520ba76f8b698e4-M4c364972705148a0eaf2eea1
Updated by Electric Monk over 1 year ago
- Status changed from New to Closed
- % Done changed from 0 to 100
git commit 19d47a18af13baff2c2fb35e9dde5bf902143f07
commit 19d47a18af13baff2c2fb35e9dde5bf902143f07 Author: Jason King <jason.brian.king@gmail.com> Date: 2021-11-09T19:17:54.000Z 13768 mpapi is too clever with its ioctls tripping SSP Reviewed by: Andy Fiddaman <andy@omnios.org> Approved by: Dan McDonald <danmcd@joyent.com>