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