Project

General

Profile

Actions

Bug #13768

closed

mpapi is too clever with its ioctls tripping SSP

Added by Jason King 7 months ago. Updated 23 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

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

issue-13768.patch (7.01 KB) issue-13768.patch Carsten Grzemba, 2021-05-04 07:16 PM
Actions

Also available in: Atom PDF