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

[Mike Small]

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