linux-kernel - Re: int overflow in io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 7 Jan 2016 10:12:02 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Benjamin LaHaise <bcrl@...ck.org>
Cc:	Jan Kara <jack@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>,
	linux-aio <linux-aio@...ck.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>
Subject: Re: int overflow in io_getevents

On Wed, Jan 6, 2016 at 7:01 PM, Benjamin LaHaise <bcrl@...ck.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@...ck.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@...ck.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/