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

[Brian C. Lane]

On Tue, Feb 12, 2019 at 04:41:47PM +0000, Mike Small wrote:
> "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.

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)