Project

General

Profile

Actions

Bug #13768

closed

mpapi is too clever with its ioctls tripping SSP

Added by Jason King about 2 years ago. Updated over 1 year ago.

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

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

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 #1

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().

Actions #2

Updated by Carsten Grzemba about 2 years ago

One possible solution would be to move the problematic data from stack to heap, like attached patch.

With this changes works mpathadm for me.

Actions #3

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.

Actions #4

Updated by Jason King about 2 years ago

  • Assignee set to Jason King
Actions #5

Updated by Electric Monk almost 2 years ago

  • Gerrit CR set to 1595
Actions #6

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.

Actions #7

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

Actions #8

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>

Actions

Also available in: Atom PDF