[PATCH] Typecasting required for comparing unlike datatypes

[PATCH] Typecasting required for comparing unlike datatypes

[Harsh Prateek Bora]

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>
---
 fs/read_write.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 431a0ed..a8eabd4 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
 	 * 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))
+	if ((pos < 0) && ( (loff_t) (pos + count) < pos))
 		return -EOVERFLOW;
 	if (file->f_mode & FMODE_UNSIGNED_OFFSET)
 		return 0;
-- 
1.7.1.1

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

Hi Al,
Any comments?

On 12/08/2010 06:25 PM, Harsh Prateek Bora 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>
> ---
>   fs/read_write.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 431a0ed..a8eabd4 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
>   	 * 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))
> +	if ((pos<  0)&&  ( (loff_t) (pos + count)<  pos))
>   		return -EOVERFLOW;
>   	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
>   		return 0;

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

On Fri, 10 Dec 2010 00:02:03 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:

> Hi Al,
> Any comments?
> 


> On 12/08/2010 06:25 PM, Harsh Prateek Bora 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>
> > ---
> >   fs/read_write.c |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 431a0ed..a8eabd4 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
> >   	 * 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))
> > +	if ((pos<  0)&&  ( (loff_t) (pos + count)<  pos))
> >   		return -EOVERFLOW;


Hmm, maybe

	if (!(file->f_mode & FMODE_UNSIGNED_OFFSET))
		return -EINVAL;
	if ((pos < 0) && (pos + count < pos))
		return -EOVERFLOW;
	return 0;

is a correct order ?

Thanks,
-Kame
	

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

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.
> 

Hmm, is this clearer ?

==

commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.

In that patch, overlap check routine is added but it was wrong.

Considering 'pos' is loff_t, a signed value,

In usual case, at comparing 'pos' and 'pos+count'

	(positive) / (positive)  OK
	(positive) / (nevative)  EOVERFLOW
	(negative) / (positive)  EINVAL
	(negative) / (negative)  EINVAL

In FMODE_UNSIGNED_OFFSET case,

	(positive) / (positive)  OK
	(positive) / (nevative)  OK (ex. 0x7fff -> 0x8000) 
	(nevative) / (negative)  OK
	(negative) / (positive)  EOVERFLOW (ex. 0xffff -> 0x1)

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 fs/read_write.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -37,11 +37,24 @@ __negative_fpos_check(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))
+	/* negative pos is allowed only when the flag is set */
+	if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+		if ((pos > 0) && (pos + count > 0))
+			return 0;
+		if ((pos > 0) && (pos + count < 0))
+			return -EOVERFLOW;
+		return -EINVAL;
+	}
+	/*
+	 * The file supports 'unsigned long' offset. (but loff_t is signed)
+	 * When pos is negative, -1 is the biggest number. So if pos + count
+	 * is larger than pos, it's overflow.
+	 * (ex) -1 + 10 = 9 ...means
+	 *    0xffff + 0xa = 0x9 => overflow.
+	 */
+	if ((pos < 0) && (pos + count > 0))
 		return -EOVERFLOW;
-	if (file->f_mode & FMODE_UNSIGNED_OFFSET)
-		return 0;
-	return -EINVAL;
+	return 0;
 }
 
 /**

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

On 12/10/2010 06:23 AM, 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.
>>
>
> Hmm, is this clearer ?
>
> ==
>
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  EOVERFLOW
> 	(negative) / (positive)  EINVAL
> 	(negative) / (negative)  EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  OK (ex. 0x7fff ->  0x8000)
> 	(nevative) / (negative)  OK
> 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->  0x1)
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
>   fs/read_write.c |   21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -37,11 +37,24 @@ __negative_fpos_check(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))
> +	/* negative pos is allowed only when the flag is set */
> +	if (!(file->f_mode&  FMODE_UNSIGNED_OFFSET)) {
> +		if ((pos>  0)&&  (pos + count>  0))
> +			return 0;
> +		if ((pos>  0)&&  (pos + count<  0))
> +			return -EOVERFLOW;
> +		return -EINVAL;
> +	}
> +	/*
> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> +	 * When pos is negative, -1 is the biggest number. So if pos + count
> +	 * is larger than pos, it's overflow.
> +	 * (ex) -1 + 10 = 9 ...means
> +	 *    0xffff + 0xa = 0x9 =>  overflow.
> +	 */
> +	if ((pos<  0)&&  (pos + count>  0))

Well, that works fine for what I am concerned but I think there is a 
mismatch in the code and the comment above. As per the comments above, 
it should be like:
			if ((pos < 0) && (pos + count > pos))

Regards,
Harsh.
>   		return -EOVERFLOW;
> -	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
> -		return 0;
> -	return -EINVAL;
> +	return 0;
>   }
>
>   /**
>
> --
> 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

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

On Fri, 10 Dec 2010 12:09:42 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
	return -EINVAL;
> > +	}
> > +	/*
> > +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> > +	 * When pos is negative, -1 is the biggest number. So if pos + count
> > +	 * is larger than pos, it's overflow.
> > +	 * (ex) -1 + 10 = 9 ...means
> > +	 *    0xffff + 0xa = 0x9 =>  overflow.
> > +	 */
> > +	if ((pos<  0)&&  (pos + count>  0))
> 
> Well, that works fine for what I am concerned but I think there is a 
> mismatch in the code and the comment above. As per the comments above, 
> it should be like:
> 			if ((pos < 0) && (pos + count > pos))
> 

Ah, yes. updated. Thank you for review and test.
-Kame
==
commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.

In that patch, overlap check routine is added but it was wrong.

Considering 'pos' is loff_t, a signed value,

In usual case, at comparing 'pos' and 'pos+count'

	(positive) / (positive)  OK
	(positive) / (nevative)  EOVERFLOW
	(negative) / (positive)  EINVAL
	(negative) / (negative)  EINVAL

In FMODE_UNSIGNED_OFFSET case,

	(positive) / (positive)  OK
	(positive) / (nevative)  OK (ex. 0x7fff -> 0x8000) 
	(nevative) / (negative)  OK
	(negative) / (positive)  EOVERFLOW (ex. 0xffff -> 0x1)

Changelog:
 - fixed a comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 fs/read_write.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -37,11 +37,24 @@ __negative_fpos_check(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))
+	/* negative pos is allowed only when the flag is set */
+	if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+		if ((pos > 0) && (pos + count > 0))
+			return 0;
+		if ((pos > 0) && (pos + count < 0))
+			return -EOVERFLOW;
+		return -EINVAL;
+	}
+	/*
+	 * The file supports 'unsigned long' offset. (but loff_t is signed)
+	 * When pos is negative, -1 is the biggest number. So if pos + count
+	 * is larger than 0, it's overflow.
+	 * (ex) -1 + 10 = 9 ...means
+	 *    0xffff + 0xa = 0x9 => overflow.
+	 */
+	if ((pos < 0) && (pos + count > 0))
 		return -EOVERFLOW;
-	if (file->f_mode & FMODE_UNSIGNED_OFFSET)
-		return 0;
-	return -EINVAL;
+	return 0;
 }
 
 /**

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> On Fri, 10 Dec 2010 12:09:42 +0530
> Harsh Bora<harsh@linux.vnet.ibm.com>  wrote:
> 	return -EINVAL;
>>> +	}
>>> +	/*
>>> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
>>> +	 * When pos is negative, -1 is the biggest number. So if pos + count
>>> +	 * is larger than pos, it's overflow.
>>> +	 * (ex) -1 + 10 = 9 ...means
>>> +	 *    0xffff + 0xa = 0x9 =>   overflow.
>>> +	 */
>>> +	if ((pos<   0)&&   (pos + count>   0))
>>
>> Well, that works fine for what I am concerned but I think there is a
>> mismatch in the code and the comment above. As per the comments above,
>> it should be like:
>> 			if ((pos<  0)&&  (pos + count>  pos))
>>
>
> Ah, yes. updated. Thank you for review and test.
> -Kame
> ==
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  EOVERFLOW
> 	(negative) / (positive)  EINVAL
> 	(negative) / (negative)  EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  OK (ex. 0x7fff ->  0x8000)
> 	(nevative) / (negative)  OK
> 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->  0x1)
>
> Changelog:
>   - fixed a comment.
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
>   fs/read_write.c |   21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -37,11 +37,24 @@ __negative_fpos_check(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))
> +	/* negative pos is allowed only when the flag is set */
> +	if (!(file->f_mode&  FMODE_UNSIGNED_OFFSET)) {
> +		if ((pos>  0)&&  (pos + count>  0))
Do we really need 2 checks? If first one is true, second one has to be 
true for count being unsigned?
> +			return 0;
> +		if ((pos>  0)&&  (pos + count<  0))
BTW, when will the above condition be true ? As if first condition is 
true, the second cant be true, as the count is unsigned.

Sorry for the lazy review ..

Regards,
Harsh


> +			return -EOVERFLOW;
> +		return -EINVAL;
> +	}
> +	/*
> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> +	 * When pos is negative, -1 is the biggest number. So if pos + count
> +	 * is larger than 0, it's overflow.
> +	 * (ex) -1 + 10 = 9 ...means
> +	 *    0xffff + 0xa = 0x9 =>  overflow.
> +	 */
> +	if ((pos<  0)&&  (pos + count>  0))
>   		return -EOVERFLOW;
> -	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
> -		return 0;
> -	return -EINVAL;
> +	return 0;
>   }
>
>   /**
>

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

On Fri, 10 Dec 2010 12:48:05 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:

> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 10 Dec 2010 12:09:42 +0530
> > Harsh Bora<harsh@linux.vnet.ibm.com>  wrote:
> > 	return -EINVAL;
> >>> +	}
> >>> +	/*
> >>> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> >>> +	 * When pos is negative, -1 is the biggest number. So if pos + count
> >>> +	 * is larger than pos, it's overflow.
> >>> +	 * (ex) -1 + 10 = 9 ...means
> >>> +	 *    0xffff + 0xa = 0x9 =>   overflow.
> >>> +	 */
> >>> +	if ((pos<   0)&&   (pos + count>   0))
> >>
> >> Well, that works fine for what I am concerned but I think there is a
> >> mismatch in the code and the comment above. As per the comments above,
> >> it should be like:
> >> 			if ((pos<  0)&&  (pos + count>  pos))
> >>
> >
> > Ah, yes. updated. Thank you for review and test.
> > -Kame
> > ==
> > commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> > negative (unsigned) page offset for very large files as /proc/<pid>/mem
> > and /dev/mem.
> >
> > In that patch, overlap check routine is added but it was wrong.
> >
> > Considering 'pos' is loff_t, a signed value,
> >
> > In usual case, at comparing 'pos' and 'pos+count'
> >
> > 	(positive) / (positive)  OK
> > 	(positive) / (nevative)  EOVERFLOW
> > 	(negative) / (positive)  EINVAL
> > 	(negative) / (negative)  EINVAL
> >
> > In FMODE_UNSIGNED_OFFSET case,
> >
> > 	(positive) / (positive)  OK
> > 	(positive) / (nevative)  OK (ex. 0x7fff ->  0x8000)
> > 	(nevative) / (negative)  OK
> > 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->  0x1)
> >
> > Changelog:
> >   - fixed a comment.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >
> > ---
> >   fs/read_write.c |   21 +++++++++++++++++----
> >   1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > Index: linux-2.6.37-rc5/fs/read_write.c
> > ===================================================================
> > --- linux-2.6.37-rc5.orig/fs/read_write.c
> > +++ linux-2.6.37-rc5/fs/read_write.c
> > @@ -37,11 +37,24 @@ __negative_fpos_check(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))
> > +	/* negative pos is allowed only when the flag is set */
> > +	if (!(file->f_mode&  FMODE_UNSIGNED_OFFSET)) {
> > +		if ((pos>  0)&&  (pos + count>  0))
Hmm.

> Do we really need 2 checks? If first one is true, second one has to be 
> true for count being unsigned?

  pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be < 0.


> > +			return 0;
> > +		if ((pos>  0)&&  (pos + count<  0))
> BTW, when will the above condition be true ? As if first condition is 
> true, the second cant be true, as the count is unsigned.
> 
Ah, hmm, type casting problem ?

	(signed) + (unsigned) => (unsigned) 

ah, ok. count should be signed...
Is this messy ?
==
commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.

In that patch, overlap check routine is added but it was wrong.

Considering 'pos' is loff_t, a signed value,

In usual case, at comparing 'pos' and 'pos+count'

	(positive) / (positive)  OK
	(positive) / (nevative)  EOVERFLOW
	(negative) / (positive)  EINVAL
	(negative) / (negative)  EINVAL

In FMODE_UNSIGNED_OFFSET case,

	(positive) / (positive)  OK
	(positive) / (nevative)  OK (ex. 0x7fff -> 0x8000) 
	(nevative) / (negative)  OK
	(negative) / (positive)  EOVERFLOW (ex. 0xffff -> 0x1)

Changelog v1->v2:
 - fixed signed+unsigned=unsigned problem.
Changelog v0->v1:
 - fixed a comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

---
 fs/read_write.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -33,15 +33,31 @@ EXPORT_SYMBOL(generic_ro_fops);
 static int
 __negative_fpos_check(struct file *file, loff_t pos, size_t count)
 {
+	ssize_t len = (ssize_t)count;
+	/* len > 0 is checked before this call. */
+	BUG_ON(len < 0);
 	/*
 	 * 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))
+	/* negative pos is allowed only when the flag is set */
+	if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+		if ((pos > 0) && (pos + len > 0))
+			return 0;
+		if ((pos > 0) && (pos + len < 0))
+			return -EOVERFLOW;
+		return -EINVAL;
+	}
+	/*
+	 * The file supports 'unsigned long' offset. (but loff_t is signed)
+	 * When pos is negative, -1 is the biggest number. So if pos + count
+	 * is larger than 0, it's overflow.
+	 * (ex) -1 + 10 = 9 ...means
+	 *    0xffff + 0xa = 0x9 => overflow.
+	 */
+	if ((pos < 0) && (pos + len > 0))
 		return -EOVERFLOW;
-	if (file->f_mode & FMODE_UNSIGNED_OFFSET)
-		return 0;
-	return -EINVAL;
+	return 0;
 }
 
 /**

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

On 12/10/2010 01:29 PM, KAMEZAWA Hiroyuki wrote:
> On Fri, 10 Dec 2010 12:48:05 +0530
> Harsh Bora<harsh@linux.vnet.ibm.com>  wrote:
>
>> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
>>> On Fri, 10 Dec 2010 12:09:42 +0530
>>> Harsh Bora<harsh@linux.vnet.ibm.com>   wrote:
>>> 	return -EINVAL;
>>>>> +	}
>>>>> +	/*
>>>>> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
>>>>> +	 * When pos is negative, -1 is the biggest number. So if pos + count
>>>>> +	 * is larger than pos, it's overflow.
>>>>> +	 * (ex) -1 + 10 = 9 ...means
>>>>> +	 *    0xffff + 0xa = 0x9 =>    overflow.
>>>>> +	 */
>>>>> +	if ((pos<    0)&&    (pos + count>    0))
>>>>
>>>> Well, that works fine for what I am concerned but I think there is a
>>>> mismatch in the code and the comment above. As per the comments above,
>>>> it should be like:
>>>> 			if ((pos<   0)&&   (pos + count>   pos))
>>>>
>>>
>>> Ah, yes. updated. Thank you for review and test.
>>> -Kame
>>> ==
>>> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
>>> negative (unsigned) page offset for very large files as /proc/<pid>/mem
>>> and /dev/mem.
>>>
>>> In that patch, overlap check routine is added but it was wrong.
>>>
>>> Considering 'pos' is loff_t, a signed value,
>>>
>>> In usual case, at comparing 'pos' and 'pos+count'
>>>
>>> 	(positive) / (positive)  OK
>>> 	(positive) / (nevative)  EOVERFLOW
>>> 	(negative) / (positive)  EINVAL
>>> 	(negative) / (negative)  EINVAL
>>>
>>> In FMODE_UNSIGNED_OFFSET case,
>>>
>>> 	(positive) / (positive)  OK
>>> 	(positive) / (nevative)  OK (ex. 0x7fff ->   0x8000)
>>> 	(nevative) / (negative)  OK
>>> 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->   0x1)
>>>
>>> Changelog:
>>>    - fixed a comment.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>
>>> ---
>>>    fs/read_write.c |   21 +++++++++++++++++----
>>>    1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> Index: linux-2.6.37-rc5/fs/read_write.c
>>> ===================================================================
>>> --- linux-2.6.37-rc5.orig/fs/read_write.c
>>> +++ linux-2.6.37-rc5/fs/read_write.c
>>> @@ -37,11 +37,24 @@ __negative_fpos_check(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))
>>> +	/* negative pos is allowed only when the flag is set */
>>> +	if (!(file->f_mode&   FMODE_UNSIGNED_OFFSET)) {
>>> +		if ((pos>   0)&&   (pos + count>   0))
> Hmm.
>
>> Do we really need 2 checks? If first one is true, second one has to be
>> true for count being unsigned?
>
>    pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be<  0.

Well, if you mean that, you need to typecast. Going back to what I 
proposed, you need to put it like that:
	if ((pos>   0)&&   ( (loff_t) (pos + count) >   0))
otherwise, the result of pos + count becomes an unsigned value on a 64 
bit system ..
>
>
>>> +			return 0;
>>> +		if ((pos>   0)&&   (pos + count<   0))
>> BTW, when will the above condition be true ? As if first condition is
>> true, the second cant be true, as the count is unsigned.
>>
> Ah, hmm, type casting problem ?
>
> 	(signed) + (unsigned) =>  (unsigned)
>
> ah, ok. count should be signed...
No, count shouldnt be signed, you may guess why. typecating the sum to 
loff_t is the solution.

Regards,
Harsh
> Is this messy ?
> ==
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  EOVERFLOW
> 	(negative) / (positive)  EINVAL
> 	(negative) / (negative)  EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> 	(positive) / (positive)  OK
> 	(positive) / (nevative)  OK (ex. 0x7fff ->  0x8000)
> 	(nevative) / (negative)  OK
> 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->  0x1)
>
> Changelog v1->v2:
>   - fixed signed+unsigned=unsigned problem.
> Changelog v0->v1:
>   - fixed a comment.
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
>   fs/read_write.c |   24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -33,15 +33,31 @@ EXPORT_SYMBOL(generic_ro_fops);
>   static int
>   __negative_fpos_check(struct file *file, loff_t pos, size_t count)
>   {
> +	ssize_t len = (ssize_t)count;
> +	/* len>  0 is checked before this call. */
> +	BUG_ON(len<  0);
>   	/*
>   	 * 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))
> +	/* negative pos is allowed only when the flag is set */
> +	if (!(file->f_mode&  FMODE_UNSIGNED_OFFSET)) {
> +		if ((pos>  0)&&  (pos + len>  0))
> +			return 0;
> +		if ((pos>  0)&&  (pos + len<  0))
> +			return -EOVERFLOW;
> +		return -EINVAL;
> +	}
> +	/*
> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> +	 * When pos is negative, -1 is the biggest number. So if pos + count
> +	 * is larger than 0, it's overflow.
> +	 * (ex) -1 + 10 = 9 ...means
> +	 *    0xffff + 0xa = 0x9 =>  overflow.
> +	 */
> +	if ((pos<  0)&&  (pos + len>  0))
>   		return -EOVERFLOW;
> -	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
> -		return 0;
> -	return -EINVAL;
> +	return 0;
>   }
>
>   /**
>
>

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

On Fri, 10 Dec 2010 13:43:26 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:

> On 12/10/2010 01:29 PM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 10 Dec 2010 12:48:05 +0530
> > Harsh Bora<harsh@linux.vnet.ibm.com>  wrote:
> >
> >> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> >>> On Fri, 10 Dec 2010 12:09:42 +0530
> >>> Harsh Bora<harsh@linux.vnet.ibm.com>   wrote:
> >>> 	return -EINVAL;
> >>>>> +	}
> >>>>> +	/*
> >>>>> +	 * The file supports 'unsigned long' offset. (but loff_t is signed)
> >>>>> +	 * When pos is negative, -1 is the biggest number. So if pos + count
> >>>>> +	 * is larger than pos, it's overflow.
> >>>>> +	 * (ex) -1 + 10 = 9 ...means
> >>>>> +	 *    0xffff + 0xa = 0x9 =>    overflow.
> >>>>> +	 */
> >>>>> +	if ((pos<    0)&&    (pos + count>    0))
> >>>>
> >>>> Well, that works fine for what I am concerned but I think there is a
> >>>> mismatch in the code and the comment above. As per the comments above,
> >>>> it should be like:
> >>>> 			if ((pos<   0)&&   (pos + count>   pos))
> >>>>
> >>>
> >>> Ah, yes. updated. Thank you for review and test.
> >>> -Kame
> >>> ==
> >>> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> >>> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> >>> and /dev/mem.
> >>>
> >>> In that patch, overlap check routine is added but it was wrong.
> >>>
> >>> Considering 'pos' is loff_t, a signed value,
> >>>
> >>> In usual case, at comparing 'pos' and 'pos+count'
> >>>
> >>> 	(positive) / (positive)  OK
> >>> 	(positive) / (nevative)  EOVERFLOW
> >>> 	(negative) / (positive)  EINVAL
> >>> 	(negative) / (negative)  EINVAL
> >>>
> >>> In FMODE_UNSIGNED_OFFSET case,
> >>>
> >>> 	(positive) / (positive)  OK
> >>> 	(positive) / (nevative)  OK (ex. 0x7fff ->   0x8000)
> >>> 	(nevative) / (negative)  OK
> >>> 	(negative) / (positive)  EOVERFLOW (ex. 0xffff ->   0x1)
> >>>
> >>> Changelog:
> >>>    - fixed a comment.
> >>>
> >>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >>>
> >>> ---
> >>>    fs/read_write.c |   21 +++++++++++++++++----
> >>>    1 file changed, 17 insertions(+), 4 deletions(-)
> >>>
> >>> Index: linux-2.6.37-rc5/fs/read_write.c
> >>> ===================================================================
> >>> --- linux-2.6.37-rc5.orig/fs/read_write.c
> >>> +++ linux-2.6.37-rc5/fs/read_write.c
> >>> @@ -37,11 +37,24 @@ __negative_fpos_check(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))
> >>> +	/* negative pos is allowed only when the flag is set */
> >>> +	if (!(file->f_mode&   FMODE_UNSIGNED_OFFSET)) {
> >>> +		if ((pos>   0)&&   (pos + count>   0))
> > Hmm.
> >
> >> Do we really need 2 checks? If first one is true, second one has to be
> >> true for count being unsigned?
> >
> >    pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be<  0.
> 
> Well, if you mean that, you need to typecast. Going back to what I 
> proposed, you need to put it like that:
> 	if ((pos>   0)&&   ( (loff_t) (pos + count) >   0))
> otherwise, the result of pos + count becomes an unsigned value on a 64 
> bit system ..
> >
> >
> >>> +			return 0;
> >>> +		if ((pos>   0)&&   (pos + count<   0))
> >> BTW, when will the above condition be true ? As if first condition is
> >> true, the second cant be true, as the count is unsigned.
> >>
> > Ah, hmm, type casting problem ?
> >
> > 	(signed) + (unsigned) =>  (unsigned)
> >
> > ah, ok. count should be signed...
> No, count shouldnt be signed, you may guess why. typecating the sum to 
> loff_t is the solution.
> 

Ok, I acked to your patch. Thank you for explaining.

Thanks,
-Kame

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

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.

> ---
>  fs/read_write.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 431a0ed..a8eabd4 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
>  	 * 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))
> +	if ((pos < 0) && ( (loff_t) (pos + count) < pos))
>  		return -EOVERFLOW;
>  	if (file->f_mode & FMODE_UNSIGNED_OFFSET)
>  		return 0;
> -- 
> 1.7.1.1
> 
> 

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

On 12/10/2010 01:48 PM, 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.
>
Oh, but I am not annoyed, .. Thanks for the ack though ! :)

>> ---
>>   fs/read_write.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 431a0ed..a8eabd4 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
>>   	 * 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))
>> +	if ((pos<  0)&&  ( (loff_t) (pos + count)<  pos))
>>   		return -EOVERFLOW;
>>   	if (file->f_mode&  FMODE_UNSIGNED_OFFSET)
>>   		return 0;
>> --
>> 1.7.1.1
>>
>>
>

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Al Viro]

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>
---
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 */
+			return -EOVERFLOW;
+	} else if (unlikely((loff_t) (pos + count) < 0)) {
+		if (!unsigned_offsets(file))
 			return retval;
 	}
 

Re: [PATCH] Typecasting required for comparing unlike datatypes

[KAMEZAWA Hiroyuki]

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 */
> +			return -EOVERFLOW;
> +	} else if (unlikely((loff_t) (pos + count) < 0)) {
> +		if (!unsigned_offsets(file))
>  			return retval;
>  	}
>  
> 

Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora]

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