GNU bug report logs - #34392
[PATCH] Avoid sigsegv in case 2nd nilfs2 superblock magic accidently found.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 34392 in the body.
You can then email your comments to 34392 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Small <smallm <at> sdf.org>
To: bug-parted <at> gnu.org
Subject: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock magic accidently
 found.
Date: Fri, 08 Feb 2019 23:03:55 +0000

[Message part 1 (text/plain, inline)]

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

[avoid-sigsegv-nilfs2-crc32.patch (text/x-patch, inline)]

From: Michael Small <smallm <at> sdf.org>
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

Message #8 received at 34392 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Mike Small <smallm <at> sdf.org>
Cc: 34392 <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Mon, 11 Feb 2019 11:59:31 -0800

On Fri, Feb 08, 2019 at 11:03:55PM +0000, Mike Small wrote:
> 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:

Good job tracking this down! Yes, a test would be good to have, I think
this is one of those corner cases that can bite people and lead to lots
of confusion :)

>  	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;

This calculation is correct, but I find it hard to read. If you use the
same technique as it does for sectors it would be easier to understand
in the future, and I don't think the superblock size is going to change.

-- 
Brian C. Lane (PST8PDT)

Message #11 received at 34392 <at> debbugs.gnu.org (full text, mbox):

From: Mike Small <smallm <at> sdf.org>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 34392 <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Tue, 12 Feb 2019 16:41:47 +0000

"Brian C. Lane" <bcl <at> redhat.com> writes:

> On Fri, Feb 08, 2019 at 11:03:55PM +0000, Mike Small wrote:
>> 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:
>
> Good job tracking this down! Yes, a test would be good to have, I think
> this is one of those corner cases that can bite people and lead to lots
> of confusion :)

I'll start working on the tests today. Maybe I should try installing
nilfs on a partition and make sure that still works too after the patch
is in good shape.

>
>>  	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;
>
> This calculation is correct, but I find it hard to read. If you use the
> same technique as it does for sectors it would be easier to understand
> in the future, and I don't think the superblock size is going to change.

Probably I should have spent more time trying to understand the way
sectors was calculated or asked about it before submitting the patch. It
confused me, since in my case, where geom->dev->sector_size was 512,
that calculation gave a size that meant eight 512 byte sectors were read
instead of two (sizeof nilfs2_super_block = 1024):

(4096 + 512 - 1) / 512 = 8.

And that's what it did, except all at once, based on the strace...

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

And then there was the 1024 offset introduced when assigning to the
primary superblock, sb, which I didn't understand the purpose of...

	if (ped_geometry_read(geom, buf, 0, sectors))
		sb = (struct nilfs2_super_block *)(buf+1024);


I wasn't sure if reading the extra six sectors for the 2nd superblock
would be okay, e.g. if the superblock was really close to the end of a
disk. And in general there are these things about reading the first
superblock which I don't understand, so I'm unclear if the two lengths
should be computed the same way. If so should we look for the 2nd
superblock 1024 bytes into the 4096 bytes read like we do for the 1st
superblock?


-- 
Mike Small
smallm <at> sdf.org

Message #14 received at 34392 <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: Mike Small <smallm <at> sdf.org>
Cc: 34392 <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Tue, 12 Feb 2019 09:56:02 -0800

On Tue, Feb 12, 2019 at 04:41:47PM +0000, Mike Small wrote:
> "Brian C. Lane" <bcl <at> redhat.com> writes:
> 
> > On Fri, Feb 08, 2019 at 11:03:55PM +0000, Mike Small wrote:
> >> 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:
> >
> > Good job tracking this down! Yes, a test would be good to have, I think
> > this is one of those corner cases that can bite people and lead to lots
> > of confusion :)
> 
> I'll start working on the tests today. Maybe I should try installing
> nilfs on a partition and make sure that still works too after the patch
> is in good shape.

That's probably a good idea.

> 
> >
> >>  	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;
> >
> > This calculation is correct, but I find it hard to read. If you use the
> > same technique as it does for sectors it would be easier to understand
> > in the future, and I don't think the superblock size is going to change.
> 
> Probably I should have spent more time trying to understand the way
> sectors was calculated or asked about it before submitting the patch. It
> confused me, since in my case, where geom->dev->sector_size was 512,
> that calculation gave a size that meant eight 512 byte sectors were read
> instead of two (sizeof nilfs2_super_block = 1024):
> 
> (4096 + 512 - 1) / 512 = 8.
> 
> And that's what it did, except all at once, based on the strace...
> 
> 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
> 
> And then there was the 1024 offset introduced when assigning to the
> primary superblock, sb, which I didn't understand the purpose of...
> 
> 	if (ped_geometry_read(geom, buf, 0, sectors))
> 		sb = (struct nilfs2_super_block *)(buf+1024);
> 
> 
> I wasn't sure if reading the extra six sectors for the 2nd superblock
> would be okay, e.g. if the superblock was really close to the end of a
> disk. And in general there are these things about reading the first
> superblock which I don't understand, so I'm unclear if the two lengths
> should be computed the same way. If so should we look for the 2nd
> superblock 1024 bytes into the 4096 bytes read like we do for the 1st
> superblock?

I can't seem to find a decent reference for NILFS other than this code
and the linux kernel code so I'm not sure why it reads so much for the
first one. I think you've got the logic right, I just think it would be
easier to read as:

sectors2 = (1024 + geom->dev->sector_size - 1) / geom->dev->sector_size;

When reading the 2nd superblock it looks like it starts on a sector
boundary so that's why it doesn't need the 4096 offset.

-- 
Brian C. Lane (PST8PDT)

Message #17 received at 34392 <at> debbugs.gnu.org (full text, mbox):

From: Mike Small <smallm <at> sdf.org>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 34392 <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Wed, 13 Feb 2019 21:33:35 +0000

[Message part 1 (text/plain, inline)]

"Brian C. Lane" <bcl <at> redhat.com> writes:

> On Tue, Feb 12, 2019 at 04:41:47PM +0000, Mike Small wrote:
>> "Brian C. Lane" <bcl <at> redhat.com> writes:
>> 
>> > On Fri, Feb 08, 2019 at 11:03:55PM +0000, Mike Small wrote:
>> >> 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:
>> >
>> > Good job tracking this down! Yes, a test would be good to have, I think
>> > this is one of those corner cases that can bite people and lead to lots
>> > of confusion :)
>> 
>> I'll start working on the tests today. Maybe I should try installing
>> nilfs on a partition and make sure that still works too after the patch
>> is in good shape.
>
> That's probably a good idea.

I've attached a patch with a test case that fails without the fix. I've
also included a nearly identical test that tries to check the case where
the s_bytes field has a reasonable value but one that should make the
crc32 check run over bytes beyond the end of what alloca set aside on the
stack. Unfortunately, reading that far out on the stack doesn't seem to
run into any problem, so the test passed even without the fix. Maybe you
won't want that test.

When I looked more closely at your tests I saw that t1700 tests
detection of a nilfs2 filesystem as long as it's run with
PARTED_SECTOR_SIZE=512. That test passed with or without this fix.

I'll send the corrected fix in a separate email.


-- 
Mike Small
smallm <at> sdf.org

[nilfs_badsb_tests.patch (text/x-patch, inline)]

From ed615cc16748c6e77ce0f859eab5cff9599feaaf Mon Sep 17 00:00:00 2001
From: Michael Small <smallm <at> sdf.org>
Date: Wed, 13 Feb 2019 16:05:21 -0500
Subject: [PATCH] Tests case for sigsegv when false nilfs2 superblock detected.

---
 tests/Makefile.am                |  2 ++
 tests/t4301-nilfs2-badsb2.sh     | 43 ++++++++++++++++++++++++++++++++++++
 tests/t4302-nilfs2-lessbadsb2.sh | 47 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100755 tests/t4301-nilfs2-badsb2.sh
 create mode 100755 tests/t4302-nilfs2-lessbadsb2.sh

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3fa75a9..0d7c022 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -66,6 +66,8 @@ TESTS = \
   t4100-msdos-starting-sector.sh \
   t4200-partprobe.sh \
   t4300-nilfs2-tiny.sh \
+  t4301-nilfs2-badsb2.sh \
+  t4302-nilfs2-lessbadsb2.sh \
   t5000-tags.sh \
   t6000-dm.sh \
   t6001-psep.sh \
diff --git a/tests/t4301-nilfs2-badsb2.sh b/tests/t4301-nilfs2-badsb2.sh
new file mode 100755
index 0000000..cef8a9a
--- /dev/null
+++ b/tests/t4301-nilfs2-badsb2.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+# Trigger a nilfs2-related bug.
+
+# Copyright (C) 2011-2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../parted
+ss=$sector_size_
+len=32
+dev=dev-file
+
+dd if=/dev/zero of=$dev bs=512 count=$(($len+$ss/512)) || framework_failure_
+
+end=$(($len * 512 / $ss))
+parted -s $dev mklabel msdos mkpart primary 1s ${end}s || framework_failure_
+
+# Write a secondary superblock with the nilfs magic number and a nilfs
+# superblock length (s_bytes) field of only 10 bytes.
+# struct nilfs2_super_block starts with these four fields...
+#	uint32_t	s_rev_level;
+#	uint16_t	s_minor_rev_level;
+#	uint16_t	s_magic;
+#	uint16_t	s_bytes;
+sb2_offset=$(( 24 / ($ss / 512) + 1))
+perl -e "print pack 'LSSS.', 0, 0, 0x3434, 10, $ss" |
+    dd of=$dev bs=$ss seek=$sb2_offset count=1 conv=notrunc
+
+# This used to give parted a sigsegv.
+parted -s $dev print || fail=1
+
+Exit $fail
diff --git a/tests/t4302-nilfs2-lessbadsb2.sh b/tests/t4302-nilfs2-lessbadsb2.sh
new file mode 100755
index 0000000..a46dccf
--- /dev/null
+++ b/tests/t4302-nilfs2-lessbadsb2.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+# Trigger a nilfs2-related bug.
+
+# Copyright (C) 2011-2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test is like t4301-nilfsbadsb2 except with an s_bytes field of
+# 1024 instead of 10. This exercises a less obvious bug.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../parted
+ss=$sector_size_
+len=32
+dev=dev-file
+
+dd if=/dev/zero of=$dev bs=512 count=$(($len+$ss/512)) || framework_failure_
+
+end=$(($len * 512 / $ss))
+parted -s $dev mklabel msdos mkpart primary 1s ${end}s || framework_failure_
+
+# Write a secondary superblock with the nilfs magic number and a nilfs
+# superblock length (s_bytes) field of only 10 bytes.
+# struct nilfs2_super_block starts with these four fields...
+#	uint32_t	s_rev_level;
+#	uint16_t	s_minor_rev_level;
+#	uint16_t	s_magic;
+#	uint16_t	s_bytes;
+sb2_offset=$(( 24 / ($ss / 512) + 1))
+perl -e "print pack 'LSSS.', 0, 0, 0x3434, 1024, $ss" |
+    dd of=$dev bs=$ss seek=$sb2_offset count=1 conv=notrunc
+
+# This used to read past the part of the stack allocated by alloca, but
+# may or may not cause a segmentation fault as a result.
+parted -s $dev print || fail=1
+
+Exit $fail
-- 
2.7.4

Message #20 received at 34392 <at> debbugs.gnu.org (full text, mbox):

From: Mike Small <smallm <at> sdf.org>
To: "Brian C. Lane" <bcl <at> redhat.com>
Cc: 34392 <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Wed, 13 Feb 2019 21:34:44 +0000

[Message part 1 (text/plain, inline)]

"Brian C. Lane" <bcl <at> redhat.com> writes:
>> >>  	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;
>> >
>> > This calculation is correct, but I find it hard to read. If you use the
>> > same technique as it does for sectors it would be easier to understand
>> > in the future, and I don't think the superblock size is going to change.
>> 
>> Probably I should have spent more time trying to understand the way
>> sectors was calculated or asked about it before submitting the patch. It
>> confused me, since in my case, where geom->dev->sector_size was 512,
>> that calculation gave a size that meant eight 512 byte sectors were read
>> instead of two (sizeof nilfs2_super_block = 1024):
>> 
>> (4096 + 512 - 1) / 512 = 8.
>> 
>> And that's what it did, except all at once, based on the strace...
>> 
>> 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
>> 
>> And then there was the 1024 offset introduced when assigning to the
>> primary superblock, sb, which I didn't understand the purpose of...
>> 
>> 	if (ped_geometry_read(geom, buf, 0, sectors))
>> 		sb = (struct nilfs2_super_block *)(buf+1024);
>> 
>> 
>> I wasn't sure if reading the extra six sectors for the 2nd superblock
>> would be okay, e.g. if the superblock was really close to the end of a
>> disk. And in general there are these things about reading the first
>> superblock which I don't understand, so I'm unclear if the two lengths
>> should be computed the same way. If so should we look for the 2nd
>> superblock 1024 bytes into the 4096 bytes read like we do for the 1st
>> superblock?
>
> I can't seem to find a decent reference for NILFS other than this code
> and the linux kernel code so I'm not sure why it reads so much for the
> first one. I think you've got the logic right, I just think it would be
> easier to read as:
>
> sectors2 = (1024 + geom->dev->sector_size - 1) / geom->dev->sector_size;
>
> When reading the 2nd superblock it looks like it starts on a sector
> boundary so that's why it doesn't need the 4096 offset.

I've attached a corrected fix with that calculation written more clearly
as you suggest.

-- 
Mike Small
smallm <at> sdf.org

[corrected_nilfs_badsb.patch (text/x-patch, inline)]

From 3c3b926e589ca2b2e03450bcdee42765b887e697 Mon Sep 17 00:00:00 2001
From: Michael Small <smallm <at> sdf.org>
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.
---
 libparted/fs/nilfs2/nilfs2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libparted/fs/nilfs2/nilfs2.c b/libparted/fs/nilfs2/nilfs2.c
index b42a464..52f757c 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 = (1024 + geom->dev->sector_size -1 ) /
+			      geom->dev->sector_size;
+	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

Message #25 received at 34392-done <at> debbugs.gnu.org (full text, mbox):

From: "Brian C. Lane" <bcl <at> redhat.com>
To: 34392-done <at> debbugs.gnu.org
Subject: Re: bug#34392: [PATCH] Avoid sigsegv in case 2nd nilfs2 superblock
 magic accidently found.
Date: Tue, 9 Apr 2019 15:54:59 -0700

THIS is the patchset pushed to master and in Fedora's parted-3.2-41
build for rawhide. Accidentally closed the wrong one earlier.


-- 
Brian C. Lane (PST8PDT)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.