On 12/16/2010 05:54 AM, KAMEZAWA Hiroyuki wrote:
On Wed, 15 Dec 2010 09:50:24 +0000
Al Viro<viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>
This seems much easier to read. thank you.
Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
