Re: int overflow in io_getevents

[Dmitry Vyukov]

On Wed, Dec 16, 2015 at 1:56 PM, Jan Kara <jack@xxxxxxx> wrote:
> On Mon 07-12-15 11:27:07, Dmitry Vyukov wrote:
>> Hello,
>>
>> While running syzkaller fuzzer on commit
>> 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8, I've hit the following UBSAN
>> warning. I think it can lead to an unexpected active wait loop, if
>> user-space expects such io_getevents to wait for a long duration but
>> instead it returns immediately, so user-space reissues the same call
>> again and again. Andrey suggested that read_events should validate
>> timeout with timespec_valid_strict before using it.
>
> Yup, looks correct. Will you send a patch?

I've drafted the verification:

@@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long
min_nr, long nr,

                if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
                        return -EFAULT;
+               if (!timespec_valid_strict(&strict))
+                       return -EINVAL;

                until = timespec_to_ktime(ts);
        }

But now I am thinking whether it is the right solution.
First, user does not know about KTIME_MAX, so it is not unreasonable
to pass timespec{INT64_MAX, INT64_MAX} as timeout expecting that it
will block for a long time. And it actually probably mostly works now,
because after the overflow you still get something large with high
probability. If we do the fix, then users will need to pass seconds <
KTIME_MAX, while they don't know KTIME_MAX value.
Second, there seems to be more serious issue in ktime_set() which
checks seconds for KTIME_MAX, but on the next line addition still
overflows int64.
Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/