Bug #13324
closed
struct dk_minfo_ext size differences trigger SSP in libfdisk
100%
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
Updated by Jason King over 2 years 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
Updated by Joshua M. Clulow over 2 years ago
- Related to Feature #13274: enable -fstack-protector-strong by default in user land added
Updated by Robert Mustacchi over 2 years 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.
Updated by Electric Monk over 2 years 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>