Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302952 > unrolled thread

Re: int overflow in io_getevents

Started byBenjamin LaHaise <bcrl@kvack.org>
First post2016-01-06 19:20 +0100
Last post2016-01-07 17:30 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: int overflow in io_getevents Benjamin LaHaise <bcrl@kvack.org> - 2016-01-06 19:20 +0100
    Re: int overflow in io_getevents Dmitry Vyukov <dvyukov@google.com> - 2016-01-07 10:20 +0100
      Re: int overflow in io_getevents Benjamin LaHaise <bcrl@kvack.org> - 2016-01-07 16:40 +0100
        Re: int overflow in io_getevents Dmitry Vyukov <dvyukov@google.com> - 2016-01-07 16:40 +0100
          Re: int overflow in io_getevents Benjamin LaHaise <bcrl@kvack.org> - 2016-01-07 17:00 +0100
            Re: int overflow in io_getevents Dmitry Vyukov <dvyukov@google.com> - 2016-01-07 17:30 +0100

#1302952 — Re: int overflow in io_getevents

FromBenjamin LaHaise <bcrl@kvack.org>
Date2016-01-06 19:20 +0100
SubjectRe: int overflow in io_getevents
Message-ID<qO0QG-4Ih-15@gated-at.bofh.it>
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@kvack.org>
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@kvack.org>

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);
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1303408

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-07 10:20 +0100
Message-ID<qOeTF-65o-37@gated-at.bofh.it>
In reply to#1302952
On Wed, Jan 6, 2016 at 7:01 PM, Benjamin LaHaise <bcrl@kvack.org> 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@kvack.org>
> 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@kvack.org>
>
> 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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303666

FromBenjamin LaHaise <bcrl@kvack.org>
Date2016-01-07 16:40 +0100
Message-ID<qOkPn-1zT-3@gated-at.bofh.it>
In reply to#1303408
On Thu, Jan 07, 2016 at 10:12:02AM +0100, Dmitry Vyukov wrote:
...
> Sorry, but the following program still prints -9223372036562067969. I
> think timespec_valid check will do.

Ah, right.  Yes, using timespec_valid() instead of  timespec_valid_strict() 
as initially proposed will address my concerns.  Updated commit below.

		-ben
-- 
"Thought is the essence of where you are now."

commit 3a55c535cf3836257434518bd6bc11464c493492
Author: Benjamin LaHaise <bcrl@kvack.org>
Date:   Thu Jan 7 10:25:44 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
    an invalid 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 timespec_valid() checks fail.  We
    use timespec_valid() instead of timespec_valid_strict() to avoid issues
    caused by userspace not knowing the cutoff for KTIME_SEC_MAX.
    
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index 155f842..d161a2f 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 (!timespec_valid())
+			return -EINVAL;
 
 		until = timespec_to_ktime(ts);
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303667

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-07 16:40 +0100
Message-ID<qOkPo-1zT-5@gated-at.bofh.it>
In reply to#1303666
On Thu, Jan 7, 2016 at 4:31 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> On Thu, Jan 07, 2016 at 10:12:02AM +0100, Dmitry Vyukov wrote:
> ...
>> Sorry, but the following program still prints -9223372036562067969. I
>> think timespec_valid check will do.
>
> Ah, right.  Yes, using timespec_valid() instead of  timespec_valid_strict()
> as initially proposed will address my concerns.  Updated commit below.
>
>                 -ben
> --
> "Thought is the essence of where you are now."
>
> commit 3a55c535cf3836257434518bd6bc11464c493492
> Author: Benjamin LaHaise <bcrl@kvack.org>
> Date:   Thu Jan 7 10:25:44 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
>     an invalid 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 timespec_valid() checks fail.  We
>     use timespec_valid() instead of timespec_valid_strict() to avoid issues
>     caused by userspace not knowing the cutoff for KTIME_SEC_MAX.
>
>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>     Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 155f842..d161a2f 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 (!timespec_valid())

pass ts to the function

> +                       return -EINVAL;
>
>                 until = timespec_to_ktime(ts);
>         }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303692

FromBenjamin LaHaise <bcrl@kvack.org>
Date2016-01-07 17:00 +0100
Message-ID<qOl8K-1I5-5@gated-at.bofh.it>
In reply to#1303667
On Thu, Jan 07, 2016 at 04:37:43PM +0100, Dmitry Vyukov wrote:
> pass ts to the function

Yeah, I should have had my morning coffee before hitting send.  Updated 
below, and hopefully final.  Checked with a test program to confirm that 
the huge value of seconds in timespec correctly waits, and that negative 
or other invalid values fail with EINVAL (download from
http://www.kvack.org/~bcrl/aio-io_getevents-timespec.c ).

		-ben
-- 
"Thought is the essence of where you are now."

commit 49b78150bc5762c58cfb8b19a859c354cf1a71ac
Author: Benjamin LaHaise <bcrl@kvack.org>
Date:   Thu Jan 7 10:37:58 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
    an invalid 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 timespec_valid() checks fail.  We
    use timespec_valid() instead of timespec_valid_strict() to avoid issues
    caused by userspace not knowing the cutoff for KTIME_SEC_MAX.
    
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index 155f842..e0d5398 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 (!timespec_valid(&ts))
+			return -EINVAL;
 
 		until = timespec_to_ktime(ts);
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303725

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-07 17:30 +0100
Message-ID<qOlBO-29Y-33@gated-at.bofh.it>
In reply to#1303692
On Thu, Jan 7, 2016 at 4:52 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> On Thu, Jan 07, 2016 at 04:37:43PM +0100, Dmitry Vyukov wrote:
>> pass ts to the function
>
> Yeah, I should have had my morning coffee before hitting send.  Updated
> below, and hopefully final.  Checked with a test program to confirm that
> the huge value of seconds in timespec correctly waits, and that negative
> or other invalid values fail with EINVAL (download from
> http://www.kvack.org/~bcrl/aio-io_getevents-timespec.c ).
>
>                 -ben
> --
> "Thought is the essence of where you are now."


Acked-by: Dmitry Vyukov <dvyukov@google.com>

Thanks!

> commit 49b78150bc5762c58cfb8b19a859c354cf1a71ac
> Author: Benjamin LaHaise <bcrl@kvack.org>
> Date:   Thu Jan 7 10:37:58 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
>     an invalid 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 timespec_valid() checks fail.  We
>     use timespec_valid() instead of timespec_valid_strict() to avoid issues
>     caused by userspace not knowing the cutoff for KTIME_SEC_MAX.
>
>     Reported-by: Dmitry Vyukov <dvyukov@google.com>
>     Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 155f842..e0d5398 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 (!timespec_valid(&ts))
> +                       return -EINVAL;
>
>                 until = timespec_to_ktime(ts);
>         }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web