linux-kernel - Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5777D335.2060205@osg.samsung.com>
Date:	Sat, 02 Jul 2016 15:44:05 +0100
From:	Luis de Bethencourt <luisbg@....samsung.com>
To:	Salah Triki <salah.triki@....org>
CC:	Salah Triki <salah.triki@...il.com>, akpm@...ux-foundation.org,
	viro@...iv.linux.org.uk, mhocko@...e.com, vdavydov@...tuozzo.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct

On 02/07/16 14:34, Salah Triki wrote:
> On Sat, Jul 02, 2016 at 12:38:18PM +0100, Luis de Bethencourt wrote:
>> On 02/07/16 09:05, Salah Triki wrote:
>>> The only caller of befs_find_brun_direct is befs_fblock2brun, which
>>> already validates that the block is within the range of direct blocks.
>>> So remove the duplicate validation.
>>>
>>> Signed-off-by: Salah Triki <salah.triki@...il.com>
>>> ---
>>>  fs/befs/datastream.c | 8 --------
>>>  1 file changed, 8 deletions(-)
>>>
>>> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
>>> index 26cc417..e224b9a 100644
>>> --- a/fs/befs/datastream.c
>>> +++ b/fs/befs/datastream.c
>>> @@ -249,17 +249,9 @@ befs_find_brun_direct(struct super_block *sb, const befs_data_stream *data,
>>>  	int i;
>>>  	const befs_block_run *array = data->direct;
>>>  	befs_blocknr_t sum;
>>> -	befs_blocknr_t max_block =
>>> -	    data->max_direct_range >> BEFS_SB(sb)->block_shift;
>>>  
>>>  	befs_debug(sb, "---> %s, find %lu", __func__, (unsigned long)blockno);
>>>  
>>> -	if (blockno > max_block) {
>>> -		befs_error(sb, "%s passed block outside of direct region",
>>> -			   __func__);
>>> -		return BEFS_ERR;
>>> -	}
>>> -
>>>  	for (i = 0, sum = 0; i < BEFS_NUM_DIRECT_BLOCKS;
>>>  	     sum += array[i].len, i++) {
>>>  		if (blockno >= sum && blockno < sum + (array[i].len)) {
>>>
>>
>> Hi Salah,
>>
>> These aren't the same check though. If we ignore the BEFS_SB(sb)->block_shift just to
>> comparing them, we can consider the checks to be the following.
>>
>> In befs_fblock2brun():
>> if (fblock < data->max_direct_range)
>>
>> In befs_find_brun_direct():
>> if (fblock > data->max_direct_range)
>>
>> Notice how one checks if the block is past the range, and the other checks if it isn't
>> before it.
>>
>> They also looked similar to me the first time I saw them and I had to double-check :)
>>
>> Thanks,
>> Luis
>>
>>
> 
> the two checks could not be evaluated to true at the same time, and since befs_find_brun_direct 
> is called only when the first check is passed, the second check will be always evaluated to false.
> So I think the second check is useless, am I right ? 
> 
> Salah
> 

You are correct!

befs_find_brun_direct() is called when the block position inside the file is inside the direct
range. Rechecking if it is past the range again inside that function, when we know it will always be
false is redundant.

Sorry for misreading it before.

The following comment on befs_find_brun_indirect() should be removed once this patch is merged, or
add the removal to this patch if you like:
-       XXX:
-       Really should check to make sure blockno is inside indirect
-       region.
-

I don't see any other caller of these three functions (_direct, _indirect, _dblindrect) being added.

Acked-by: Luis de Bethencourt <luisbg@....samsung.com>

Thanks :)
Luis