Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora <harsh@linux.vnet.ibm.com>]

On 12/16/2010 05:54 AM, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 09:50:24 +0000
> Al Viro<viro@ZenIV.linux.org.uk>  wrote:
>
>> On Fri, Dec 10, 2010 at 05:18:51PM +0900, KAMEZAWA Hiroyuki wrote:
>>> On Wed,  8 Dec 2010 18:25:00 +0530
>>> Harsh Prateek Bora<harsh@linux.vnet.ibm.com>  wrote:
>>>
>>>> The existing code causes the if condition to pass when it should fail
>>>> on a *64-bit kernel* because of implicit data type conversions. It can
>>>> be observed by passing pos = -1 and count = some positive number.
>>>> This results in function returning EOVERFLOW instead of EINVAL.
>>>>
>>>> With this patch, the function returns EINVAL when pos is -1 and count
>>>> is a positive number. This can be tested by calling sendfile with
>>>> offset = -1 and count = some positive number on a 64-bit kernel.
>>>>
>>>> Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
>>>
>>> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>
>>> I'm sorry for annoying you.
>>
>> NAK.  Look at what's getting done there; this check is the _only_ place
>> in function that uses count, it is constantly false if count is zero
>> _and_ we have only one caller that passes non-zero.  Moreover, in case of
>> count being zero we ignore offset as well.
>>
>> Obvious things to do:
>> 	a) take the check in question into the only caller that would
>> pass non-zero count (i.e. rw_verify_area())
>> 	b) lose the second and the third arguments of __negative_fpos_check()
>> 	c) sort out what the hell should be done in that place.
>>
>> Note that current logics is very convoluted.  First of all, we rely on
>> loff_t being long long in the kernel and size_t being not bigger than
>> unsigned long long.  See ((loff_t)(pos + count)<  0) in there - if count
>> gets wider than pos, we suddenly get all kinds of odd crap.
>>
>> That's OK; if we run into ports with size_t wider than 64 bits, we'll have
>> more trouble anyway.
>>
>> So what we have for signed offset case is pos>= 0, count<= max loff_t - pos.
>> Fair enough.  For unsigned offset case it's more interesting - we want to
>> allow pos<  0, and we just want to prohibit wraparounds from negative to
>> positive.  IOW, it's pos>= 0 || count<  -pos; note that we already have
>> count<= max ssize_t, which we assume to be no more than max loff_t.
>>
>> So what we need is this:
>> 	if (unlikely(pos<  0)) {
>> 		if it's signed
>> 			-EINVAL
>> 		if (count>= -pos) /* both values are in 0..LLONG_MAX */
>> 			-EOVERFLOW
>> 	} else if (unlikely((loff_t)(pos + count)<  0)) {
>> 		if it's signed
>> 			 -EINVAL
>> 	}
>> and we are done with that.  IOW, how about the patch below?
>>
>>
>> Signed-off-by: Al Viro<viro@zeniv.linux.org.uk>
>
> This seems much easier to read. thank you.
>
> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> and sorry for my 1st implemenation.
>
>> ---
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 5d431ba..5520f8a 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -30,18 +30,9 @@ const struct file_operations generic_ro_fops = {
>>
>>   EXPORT_SYMBOL(generic_ro_fops);
>>
>> -static int
>> -__negative_fpos_check(struct file *file, loff_t pos, size_t count)
>> +static inline int unsigned_offsets(struct file *file)
>>   {
>> -	/*
>> -	 * pos or pos+count is negative here, check overflow.
>> -	 * too big "count" will be caught in rw_verify_area().
>> -	 */
>> -	if ((pos<  0)&&  (pos + count<  pos))
>> -		return -EOVERFLOW;
>> -	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
>> -		return 0;
>> -	return -EINVAL;
>> +	return file->f_mode&  FMODE_UNSIGNED_OFFSET;
>>   }
>>
>>   /**
>> @@ -75,7 +66,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
>>   		break;
>>   	}
>>
>> -	if (offset<  0&&  __negative_fpos_check(file, offset, 0))
>> +	if (offset<  0&&  !unsigned_offsets(file))
>>   		return -EINVAL;
>>   	if (offset>  inode->i_sb->s_maxbytes)
>>   		return -EINVAL;
>> @@ -152,7 +143,7 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
>>   			offset += file->f_pos;
>>   	}
>>   	retval = -EINVAL;
>> -	if (offset>= 0 || !__negative_fpos_check(file, offset, 0)) {
>> +	if (offset>= 0 || unsigned_offsets(file)) {
>>   		if (offset != file->f_pos) {
>>   			file->f_pos = offset;
>>   			file->f_version = 0;
>> @@ -252,9 +243,13 @@ int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count
>>   	if (unlikely((ssize_t) count<  0))
>>   		return retval;
>>   	pos = *ppos;
>> -	if (unlikely((pos<  0) || (loff_t) (pos + count)<  0)) {
>> -		retval = __negative_fpos_check(file, pos, count);
>> -		if (retval)
>> +	if (unlikely(pos<  0)) {
>> +		if (!unsigned_offsets(file))
>> +			return retval;
>> +		if (count>= -pos) /* both values are in 0..LLONG_MAX */
Yeah .. thats a better reorg of code, however I am not sure if we need a 
typecast above for future portability reasons (as count and pos can be 
of diff width somewhere/sometime). Also, I personally would like to keep 
the function name as is_offset_unsigned().
Anyways, +1 for ACK !

Regards,
Harsh
>> +			return -EOVERFLOW;
>> +	} else if (unlikely((loff_t) (pos + count)<  0)) {
>> +		if (!unsigned_offsets(file))
>>   			return retval;
>>   	}
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html