bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock magic accidently found.

[Mike Small]

Hi,

Someone shared with me a case where parted 3.2 (3.2-15 as packaged in
Ubuntu Xenial) hit a sigsegv when run as follows:

parted -m -s /dev/sda print

When I looked into it, it appeared that they were extremely, extremely
unlucky. It's not the same nilfs problem Jim Meyering fixed back in
3.1. They just happened to have data that looked like the magic number
for a nilfs2 superblock in just the right place for parted to think
there might be a secondary nilfs superblock. So parted tried to do a
crc32 check on that sector (+ 512 more bytes beyond the end of it), but
with most of the struct being invalid in ways that led to reading beyond
the buffer allocated by alloca in nilfs2_probe().

The partition table looked like this (using sfdisk here since I don't
haven't put my fixed version of parted on the machine yet):

~# sfdisk -l /dev/sda
Disk /dev/sda: 233.8 GiB, 251059544064 bytes, 490350672 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x00000000

Device     Boot    Start       End   Sectors   Size Id Type
/dev/sda1           4096  10489855  10485760     5G 83 Linux
/dev/sda2       10489856  23072767  12582912     6G 83 Linux
/dev/sda3       23072768  60821503  37748736    18G 83 Linux
/dev/sda4       60821504 490350591 429529088 204.8G 83 Linux

The strace just before the sigsegv shows the seeks and reads, one near
the beginning and one near the end of sd3, that happen in
is_valid_nilfs_sb():

...
read(3, 
"C\16\322EC\213\234\224i(-f\365,\214\256\n\247\"x\350\0372\n0%]\242\5QJ\16"..., 
512) = 512
lseek(3, 7168, SEEK_SET)                = 7168
read(3, 
"F\241\245\35\260\263\306\7\2\211U\16\326\275ph\225\370\273\222\272Q\332\274\346\323\365\251\370f?\5"...,
 512) = 512
lseek(3, 7680, SEEK_SET)                = 7680
read(3, 
"\340\216\364*\365\347\25H\373\4|\33FQ\23\252\376tX:\345\227\342!\324(j;k-\227b"...,
 512) = 512
lseek(3, 5370806272, SEEK_SET)          = 5370806272
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
512) = 512
lseek(3, 11813388288, SEEK_SET)         = 11813388288
read(3, " \200\0\0 \200\1\0 \200\2\0 \200\3\0 \200\4\0 \200\f\0 \200\r\0 
\200\30\0"..., 512) = 512
lseek(3, 11813322752, SEEK_SET)         = 11813322752
read(3, 
"\20\200\0\0\20\200\1\0\20\200\2\0\20\200\3\0\20\200\4\0\20\200\f\0\20\200\r\0\20\200\30\0"...,
 512) = 512
lseek(3, 11813257216, SEEK_SET)         = 11813257216
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
4096) = 4096
lseek(3, 31140605952, SEEK_SET)         = 31140605952
read(3, "42 42 44\n\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 512) = 
512
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x7ffcddd26000} ---
write(2, "\n\nYou found a bug in GNU Parted!"..., 828
...

The person sent me these sectors, so I was able to create a vm with the
same layout and dd in the unfortunate sector data at byte 31140605952
(sector 60821496).  With that I reproduced the bug with gdb and saw this
stack trace. Notice the value of len passed to __efi_crc32():

(gdb) set args -s /dev/vda print
(gdb) run
Starting program: /root/parted/parted/.libs/lt-parted -s /dev/vda print
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7ba8bbe in __efi_crc32 (buf=0x7fffffffd3c4, 
len=18446744073709551606, 
    seed=2213123465) at efi_crc32.c:122
122     efi_crc32.c: No such file or directory.
(gdb) bt
#0  0x00007ffff7ba8bbe in __efi_crc32 (buf=0x7fffffffd3c4, 
len=18446744073709551606, 
    seed=2213123465) at efi_crc32.c:122
#1  0x00007ffff7b9f571 in is_valid_nilfs_sb (sb=0x7fffffffd3b0) at 
nilfs2/nilfs2.c:97
#2  0x00007ffff7b9f764 in nilfs2_probe (geom=0x61de38) at nilfs2/nilfs2.c:124
#3  0x00007ffff7b8ba4b in ped_file_system_probe_specific (
    fs_type=0x7ffff7dd20c0 <nilfs2_type>, geom=0x61de38) at filesys.c:203
#4  0x00007ffff7b8bc96 in ped_file_system_probe (geom=0x61de38) at filesys.c:273
#5  0x00007ffff7ba3614 in read_table (disk=0x61e1e0, sector=0, 
is_extended_table=0)
    at dos.c:1050
#6  0x00007ffff7ba3850 in msdos_read (disk=0x61e1e0) at dos.c:1106
#7  0x00007ffff7b8d912 in ped_disk_new (dev=0x61e130) at disk.c:200
#8  0x000000000040764e in do_print (dev=0x7fffffffeb08, diskp=0x7fffffffeb10)
    at parted.c:1067
#9  0x0000000000405346 in command_run (cmd=0x617650, dev=0x7fffffffeb08, 
    diskp=0x7fffffffeb10) at command.c:141
#10 0x000000000040ea27 in non_interactive_mode (dev=0x7fffffffeb08, 
disk=0x7fffffffeb10, 
    cmd_list=0x6146c0 <commands>, argc=1, argv=0x7fffffffec20) at ui.c:1636
#11 0x000000000040abd2 in main (argc=1, argv=0x7fffffffec20) at parted.c:2295


There were two problems I saw:

1. is_valid_nilfs_sb() should make sure the subtraction bytes - sumoff -
4 won't give a negative number. I saw 10 for bytes and 16 for sumoff and
that was why the len argument to __efi_crc32() was so strange, the
negative number being sent over to an unsigned long.

2. Not sure if you'll want to do this part differently than my patch
does, but nilfs2_probe() should read and allocate enough sectors to hold
a struct nilfs2_super_block.  is_valid_nilfs_sb() will be passing up to
1024 bytes to __efi_crc32(). If only one 512 byte sector had been
allocated with alloca and read from disk that would cause reads off the
the end of the stack even if bytes were more than sumoff - 4. This isn't
the case I saw but I think it would be a problem if s_bytes happened to be
between 508 and 1024.

I've attached a patch and tested it in my vm.  I wanted to get this out
to you before I go away for the weekend, but if you'd like me to try to
write a test I could attempt that next week, perhaps.

Regards,
Mike Small

From: Michael Small <address@hidden>
Date: Fri, 8 Feb 2019 17:01:43 -0500
Subject: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock magic accidently
 found.

1. is_valid_nilfs_sb: make sure the subtraction bytes - sumoff - 4
won't give a negative number. That as the len argument to
__efi_crc32() would give a very large number for the latter's for
loop limit, since len is unsigned long.

2. nilfs2_probe: Read and allocate enough sectors to hold a
struct nilfs2_super_block.  is_valid_nilfs_sb() will be passing
up to 1024 bytes to __efi_crc32(). If only one 512 byte sector
had been allocated with alloca and read from disk that would cause
reads off the the end of the stack even if bytes were more than
sumoff - 4.

 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libparted/fs/nilfs2/nilfs2.c b/libparted/fs/nilfs2/nilfs2.c
index b42a464..9ad1bfc 100644
--- a/libparted/fs/nilfs2/nilfs2.c
+++ b/libparted/fs/nilfs2/nilfs2.c
@@ -89,7 +89,7 @@ is_valid_nilfs_sb(struct nilfs2_super_block *sb)
                return 0;
 
        bytes = PED_LE16_TO_CPU(sb->s_bytes);
-       if (bytes > 1024)
+       if (bytes > 1024 || bytes < sumoff - 4)
                return 0;
 
        crc = __efi_crc32(sb, sumoff, PED_LE32_TO_CPU(sb->s_crc_seed));
@@ -113,11 +113,13 @@ nilfs2_probe (PedGeometry* geom)
        const int sectors = (4096 + geom->dev->sector_size - 1) /
                             geom->dev->sector_size;
        char *buf = alloca (sectors * geom->dev->sector_size);
-       void *buff2 = alloca (geom->dev->sector_size);
+       const int sectors2 = sizeof(struct nilfs2_super_block) / 
geom->dev->sector_size +
+                (sizeof(struct nilfs2_super_block) % geom->dev->sector_size == 
0) ? 0 : 1;
+       void *buff2 = alloca (sectors2 * geom->dev->sector_size);
 
        if (ped_geometry_read(geom, buf, 0, sectors))
                sb = (struct nilfs2_super_block *)(buf+1024);
-       if (ped_geometry_read(geom, buff2, sb2off, 1))
+       if (ped_geometry_read(geom, buff2, sb2off, sectors2))
                sb2 = buff2;
 
        if ((!sb || !is_valid_nilfs_sb(sb)) &&
-- 
2.7.4