Re: FFS: wrong superblock check ~> crash

[David Holland <dholland-tech%netbsd.org@localhost>]

On Mon, Oct 20, 2014 at 03:38:11PM +0200, Maxime Villard wrote:
 > I think the sanity check should be:
 > 
 > Index: ffs_vfsops.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
 > retrieving revision 1.299
 > diff -u -r1.299 ffs_vfsops.c
 > --- ffs_vfsops.c	24 May 2014 16:34:04 -0000	1.299
 > +++ ffs_vfsops.c	20 Oct 2014 13:01:46 -0000
 > @@ -974,7 +974,7 @@
 >  			continue;
 >  
 >  		/* Validate size of superblock */
 > -		if (sbsize > MAXBSIZE || sbsize < sizeof(struct fs))
 > +		if (sbsize > SBLOCKSIZE || sbsize < sizeof(struct fs))
 >  			continue;
 >  
 >  		/* Check that we can handle the file system blocksize */
 > 
 > Tested on NetBSD-current: no longer crashes.
 > 
 > Ok/Comments?

I think the check should be left alone, but afterwards the value
should be clamped to the amount of data that can actually be
transferred. Otherwise I think it may break, e.g. on volumes with odd
block sizes.

I'm not sure what gets put in sbsize in such cases; if it's always
SBLOCKSIZE, then we should probably reject anything that isn't exactly
SBLOCKSIZE and forget about slop or hypothetical compatibility with
larger superblocks. If it's the block size of the block the superblock
is in, though, then there should be a test like this.

Another semi-related thing to check, btw: does it invalidate the
buffers for candidate superblocks that reads and then rejects? Failure
to do this when the volume blocksize isn't SBLOCKSIZE (since it seems
to always read buffers of size SBLOCKSIZE here) might cause
interesting overlapping buffer lossage later.

-- 
David A. Holland
dholland%netbsd.org@localhost