On 17.06.2016 20:25, Ryusuke Konishi wrote:
> Hi,
> 
> I have a few comments.  Please see the inline comments below.
> 
> On Fri, 17 Jun 2016 10:06:45 +0200, Torsten Hilbrich wrote:
>> The value bytes comes from the filesystem which is about to be
>> mounted. We cannot trust that the value is always in the range
>> we expect it to be.
>>
>> Check its value before using it to calculate the length for the
>> crc32_le call. It value must be larger (or equal) sumoff + 4 and
>> smaller than the remaining space in the block where the superblock
>> is stored (BLOCK_SIZE - sumoff - 4).
>>
> 
>> This fixes a problem where the accidential mounting of an encrypted
>> volume caused a kernel panic. It had the correct magic value 0x3434
>> at offset 0x406 by chance and the following 2 bytes were 0x01, 0x00
>> (interpreted as s_bytes value with value 1).
> 
> Could you improve the description on the problem a bit ?
> 
> The true issue looks a "memory access overrun" on the superblock
> buffer that can occur when "s_bytes - sumoff - 4" underflows and
> crc32_le() receives an uninteded large byte count.
>
> The reason why a "kernel panic" occurs, is unclear in the comment.
>
> Even if a non-nilfs volume is mistakenly mounted as a nilfs volume, it
> usually will return an error.  Kernel panic should never occur just by
> an accidental mount.  If it happens except the overrun issue, it is
> another bug.

You are right, I only got the kernel panic when first testing on a
grsecurity kernel. I repeated the test and there just a BUG on the
underflow/overrun is reported. I have attached the kernel logs of this
test run to this mail (nilfs.bug).

> 
>>
>> Signed-off-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>> Tested-by: Torsten Hilbrich <torsten.hilbrich-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  fs/nilfs2/the_nilfs.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
>> index 69bd801..21f6b23 100644
>> --- a/fs/nilfs2/the_nilfs.c
>> +++ b/fs/nilfs2/the_nilfs.c
>> @@ -438,18 +438,19 @@ static int nilfs_valid_sb(struct nilfs_super_block *sbp)
>>  	static unsigned char sum[4];
>>  	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
>>  	size_t bytes;
>> +	size_t crc_offset = sumoff + 4;
> 
> The name "crc_offset" is confusing.  This just means the offset of the
> latter part of the region that the crc function will verify.
> In addition, it should be defined with a "const" qualifier.

I will rename it to crc_start.

> 
>>  	u32 crc;
>>  
>>  	if (!sbp || le16_to_cpu(sbp->s_magic) != NILFS_SUPER_MAGIC)
>>  		return 0;
>>  	bytes = le16_to_cpu(sbp->s_bytes);
>> -	if (bytes > BLOCK_SIZE)
> 
>> +	if (bytes < crc_offset || bytes > BLOCK_SIZE - crc_offset)
> 
> The latter part of this test should be "bytes > BLOCK_SIZE".
> Why do you subtract crc_offset here ?

I made here some wrong assumption about the memory layout and the
meaning of bytes. bytes is the end of the area which is to be
crc32-checked so comparing against BLOCK_SIZE (which should be the
maximum size of the super block) is okay.

> 
> Regards,
> Ryusuke Konishi
> 
>>  		return 0;
>>  	crc = crc32_le(le32_to_cpu(sbp->s_crc_seed), (unsigned char *)sbp,
>>  		       sumoff);
>>  	crc = crc32_le(crc, sum, 4);
>> -	crc = crc32_le(crc, (unsigned char *)sbp + sumoff + 4,
>> -		       bytes - sumoff - 4);
>> +	crc = crc32_le(crc, (unsigned char *)sbp + crc_offset,
>> +		       bytes - crc_offset);
>>  	return crc == le32_to_cpu(sbp->s_sum);
>>  }
>>  
>> -- 
>> 2.1.4
>> --

I will sent an updated patch.

	Torsten