Re: [PATCH] Typecasting required for comparing unlike datatypes

[Al Viro <viro@ZenIV.linux.org.uk>]

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;
 	}