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 */
> + 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
