On Feb 10, 2008 12:22 PM, Bastian Blank <bastian@waldi.eu.org> wrote:
quoted text
> On Sun, Feb 10, 2008 at 04:40:53AM -0500, Niki Denev wrote:
> > this fixed the problem for me (kernel 2.6.24.1) :
> > It appears that the initial patch checked the input to vmsplice_to_user,
> > but the exploit used vmsplice_to_pipe which remained open to the attack.
>
> This patch is broken. It opens the old hole again.
>
> > @@ -1450,6 +1454,31 @@
> >               .ops = &user_page_pipe_buf_ops,
> >       };
> >
> > +     error = ret = 0;
> > +
> > +     /*
> > +      * Get user address base and length for this iovec.
> > +      */
> > +     error = get_user(base, &iov->iov_base);
> > +     if (unlikely(error))
> > +             return error;
> > +     error = get_user(len, &iov->iov_len);
> > +     if (unlikely(error))
> > +             return error;
>
> iov is unchecked.
>
> > +     if (unlikely(!access_ok(VERIFY_WRITE, base, len))) {
> > +             return -EFAULT;
> > +     }
>
> Use VERIFY_READ and this only checks the first entry.
>
> I checked the following patch and it at least fixes the known exploit.
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 14e2262..80beb2b 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1237,6 +1237,11 @@ static int get_iovec_page_array(const struct iovec __user *iov,
>                 if (unlikely(!base))
>                         break;
>
> +               if (!access_ok(VERIFY_READ, base, len)) {
> +                       error = -EFAULT;
> +                       break;
> +               }
> +
>                 /*
>                  * Get this base offset and number of pages, then map
>                  * in the user pages.
> --
> Even historians fail to learn from history -- they repeat the same mistakes.
>                 -- John Gill, "Patterns of Force", stardate 2534.7
>

As far as i can see, at least on x86 and x86_64 the first argument to
access_ok : (VERIFY_READ|VERIFY_WRITE) is ignored.
Also even if it is used on different arch, using WRITE instead of READ
should be safe because WRITE is a superset of READ.

You are right that it only catches the first entry.

--Niki
--
unsubscribe notice
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/