Project

General

Profile

Bug #13324

struct dk_minfo_ext size differences trigger SSP in libfdisk

Added by Jason King 5 months ago. Updated 5 months ago.

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

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

Unfortunately, the 64-bit version of struct dk_minfo_ext has an extra 4 bytes of padding over the 32-bit version. This was discovered after the zfs tests were run on a system built with the stack smashing protection (SSP):

root@pi:/var/tmp/test_results/20201120T103646# mdb /usr/sbin/format core
Loading modules: [ libc.so.1 ld.so.1 ]
> ::sizeof struct dk_minfo_ext
sizeof (struct dk_minfo_ext) = 0x14
>
root@pi:/var/tmp/test_results/20201120T103646# mdb -k
Loading modules: [ unix genunix specfs dtrace mac cpu.generic uppc apix scsi_vhci zfs ip hook neti sockfs arp usba smbios stmf stmf_sbd mm lofs random cpc crypto ufs logindmux nsmb ptm smbsrv nfs ipc ]
> ::sizeof struct dk_minfo_ext
sizeof (struct dk_minfo_ext) = 0x18
>

From the definition of struct dk_minfo_ext, the reason is straightforward:

/*
 * Used for Media info or the current profile info
 * including physical block size if supported.
 */
struct dk_minfo_ext {
        uint_t          dki_media_type; /* Media type or profile info */
        uint_t          dki_lbsize;     /* Logical blocksize of media */
        diskaddr_t      dki_capacity;   /* Capacity as # of dki_lbsize blks */
        uint_t          dki_pbsize;     /* Physical blocksize of media */
};

For 64-bit, an extra 4 bytes of padding is appended to the end of the struct. Since there is no separate 32-bit version, all of the existing kernel drivers that implement DKIOCGMEDIAINFOEXT have been copying out an extra 4 bytes of data. This was seen in the zfs tests since the format command uses libfdisk which issues this ioctl, causing a stack similar to:

> ::stack
libc.so.1`syscall+0x13(febac144, 1c, 251f153, 0, 8036ad8, fee64000)
0xfeb2d1ff(84f75e0)
libfdisk.so.1`__stack_chk_fail_local+0x17(fee50e71, fef10018, feadac78, 84f75e0, 40, 10)
libfdisk.so.1`fdisk_init_master_part_table(84f89e8)
libfdisk.so.1`fdisk_init_master_part_table+0x26(84f89e8)
libfdisk.so.1`libfdisk_init+0x216(80370b0, 8036c6c, 0, 1)
extpart_init+0x2f(80370b0, 84f6da8, 200, feb1b2f9, fec0080c, f18570a8)
copy_solaris_part+0x109(84f5e84, 1, 0, 8066064)
init_globals+0x351(84f5e28, 84f5e28, 0, 80663da)
main+0x3a6(803781c, fec0d628, 8037858, 8057ed7, 7, 803787c)
_start_crt+0x96(7, 803787c, f182c6f1, 0, 0, 0)
_start+0x1a(7, 8037ad0, 8037ad7, 8037ada, 8037add, 8037ae0)

Related issues

Related to illumos gate - Feature #13274: enable -fstack-protector-strong by default in user landClosedRobert Mustacchi

Actions
#1

Updated by Jason King 5 months ago

  • Subject changed from struct dk_minfo_ext size differences trigger SSP protection in libfdisk to struct dk_minfo_ext size differences trigger SSP in libfdisk
#2

Updated by Jason King 5 months ago

  • Description updated (diff)
#3

Updated by Electric Monk 5 months ago

  • Gerrit CR set to 1075
#4

Updated by Joshua M. Clulow 5 months ago

  • Related to Feature #13274: enable -fstack-protector-strong by default in user land added
#5

Updated by Robert Mustacchi 5 months ago

To test this I rigged up a system that had an nvme device (blkdev), a sata device (sd), a labeled lofi device (lofi), and a zvol (zfs). For each of these I verified that format correctly read information. As I didn't have a zfs path that triggered the ioctl there, I wrote a small, dirty C program that when built with -fstack-protector-all would trigger the error on systems without the fix. I used this to verify the zvol and then again other devices:

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <err.h>
#include <sys/dkio.h>
#include <stdlib.h>
#include <unistd.h>

static int
getinfo(int fd)
{
        struct dk_minfo_ext dkmext;

        if (ioctl(fd, DKIOCGMEDIAINFOEXT, &dkmext) != 0) {
                warn("failed to ioctl");
        }

        printf("dkmext lbsize is %u\n", dkmext.dki_lbsize);

        return (0);
}

int
main(int argc, const char *argv[])
{
        if (argc != 2) {
                errx(EXIT_FAILURE, "path please");
        }
        int fd = open(argv[1], O_RDONLY);
        struct dk_minfo_ext dkmext;
        if (fd < 0) {
                err(EXIT_FAILURE, "failed to open zvol");
        }

        getinfo(fd);

        return (0);
}

With the fix in place we no longer trigger this. I also spot checked prtvtoc, prtconf, fmtopo -V, and prtdiag to make sure there weren't other regressions.

#6

Updated by Electric Monk 5 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 7b4214534ccdf6f8aa6e566d7501aab328c72e84

commit  7b4214534ccdf6f8aa6e566d7501aab328c72e84
Author: Robert Mustacchi <rm@fingolfin.org>
Date:   2020-12-01T21:50:00.000Z

    13324 struct dk_minfo_ext size differences trigger SSP in libfdisk
    Reviewed by: Jason King <jason.brian.king@gmail.com>
    Reviewed by: Andy Fiddaman <andy@omniosce.org>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Approved by: Joshua Clulow <josh@sysmgr.org>

Also available in: Atom PDF