On Wed, Jan 6, 2016 at 7:01 PM, Benjamin LaHaise <bcrl@xxxxxxxxx> wrote:
> On Wed, Dec 16, 2015 at 07:38:33PM +0100, Dmitry Vyukov wrote:
>> > 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?
>
> I finally had some time to look over this after the holidays, and I
> don't think using timespec_valid_strict() is the right approach here,
> as userspace will have no idea what KTIME_MAX is. Instead, I think the
> right approach is to -EINVAL for negative values (which should avoid
> the overflow), and to allow too large values to be silently truncated
> by timespec_to_ktime(). The truncation doesn't matter all that much
> given that it's in the hundreds of years ballpark. I'll push the patch
> below if there are no objections.
>
> -ben
> --
> "Thought is the essence of where you are now."
>
> commit 4304367826d0df42086ef24428c6c277acd822a9
> Author: Benjamin LaHaise <bcrl@xxxxxxxxx>
> Date: Wed Jan 6 12:46:12 2016 -0500
>
> aio: handle integer overflow in io_getevents() timespec usage
>
> Dmitry Vyukov reported an integer overflow in io_getevents() when
> running a fuzzer. Upon investigation, the triggers appears to be that a
> negative value for the tv_sec or tv_nsec was passed in which is not
> handled by timespec_to_ktime(). This patch fixes that by making
> io_getevents() return -EINVAL when negative timeouts are passed in.
>
> Signed-off-by: Benjamin LaHaise <bcrl@xxxxxxxxx>
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 155f842..f325ed4 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -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 ((ts.tv_sec < 0) || (ts.tv_nsec < 0))
> + return -EINVAL;
>
> until = timespec_to_ktime(ts);
> }
Sorry, but the following program still prints -9223372036562067969. I
think timespec_valid check will do.
#include <stdio.h>
#include <limits.h>
typedef long s64;
typedef unsigned long u64;
#define TIME64_MAX ((s64)~((u64)1 << 63))
#define KTIME_MAX ((s64)~((u64)1 << 63))
#define KTIME_SEC_MAX (KTIME_MAX / NSEC_PER_SEC)
#define NSEC_PER_SEC 1000000000L
#define unlikely(x) x
struct timespec {
long tv_sec; /* seconds */
long tv_nsec; /* nanoseconds */
};
union ktime {
s64 tv64;
};
typedef union ktime ktime_t;
static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
{
if (unlikely(secs >= KTIME_SEC_MAX))
return (ktime_t){ .tv64 = KTIME_MAX };
return (ktime_t) { .tv64 = secs * NSEC_PER_SEC + (s64)nsecs };
}
static inline ktime_t timespec_to_ktime(struct timespec ts)
{
return ktime_set(ts.tv_sec, ts.tv_nsec);
}
int main(void)
{
struct timespec ts = {KTIME_SEC_MAX - 1, INT_MAX};
ktime_t t;
if ((ts.tv_sec < 0) || (ts.tv_nsec < 0))
return 0;
t = timespec_to_ktime(ts);
printf("%ld\n", t.tv64);
return 0;
}
--
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
