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

[Mike Small]

"Brian C. Lane" <address@hidden> 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
address@hidden


>From 3c3b926e589ca2b2e03450bcdee42765b887e697 Mon Sep 17 00:00:00 2001
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.
---
 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