Re: FFS: wrong superblock check ~> crash

[Maxime Villard <max%M00nBSD.net@localhost>]

Le 20/10/2014 18:48, Maxime Villard a écrit :
> 
> Le 20/10/2014 18:23, David Holland a écrit :
>>
>> 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.
> 
> Yes that's what I thought first, but I saw a comment in ffs/fs.h on this:
> 
> 	"In all cases the size of the superblock will be SBLOCKSIZE."
> 
> 

I think changing

-		if (sbsize > MAXBSIZE || sbsize < sizeof(struct fs))
+		if (sbsize > SBLOCKSIZE || sbsize < sizeof(struct fs))

is ok. The kernel will behave randomly if sbsize>8192, and this bug has been
here since the initial revision (20 years ago); if there were really strange
fs'es in the wild they would have triggered a panic, and we would have ended
up knowing it.

This patch simply makes the kernel return an error instead of behaving
randomly, with no implementation change. It is the less intrusive fix.

And from a more or less official specification:

http://www.phptr.com/content/images/0131482092/samplechapter/mcdougall_ch15.pdf

Page 11:
"The primary super-block starts at an offset of 8192 bytes into the partition
 slice and occupies one file system block (usually 8192 bytes, but can be 4096
 bytes on x86 architectures)."

So 8192 should be fine.