Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191009192105.GC26530@ZenIV.linux.org.uk>
In-Reply-To: <20191009192105.GC26530@ZenIV.linux.org.uk>
From:   Max Filippov <jcmvbkbc@gmail.com>
Date:   Wed, 9 Oct 2019 18:38:12 -0700
Message-ID: <CAMo8BfKUOmExGRMaUPmcRsy=iyRrguLF6JOLUMegNnzkF9vcvQ@mail.gmail.com>
Subject: Re: [PATCH] xtensa: fix {get,put}_user() for 64bit values
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     "open list:TENSILICA XTENSA PORT (xtensa)" 
        <linux-xtensa@linux-xtensa.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 9, 2019 at 12:21 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> First of all, on short copies __copy_{to,from}_user() return the amount
> of bytes left uncopied, *not* -EFAULT.  get_user() and put_user() are
> expected to return -EFAULT on failure.
>
> Another problem is get_user(v32, (__u64 __user *)p); that should
> fetch 64bit value and the assign it to v32, truncating it in process.
> Current code, OTOH, reads 8 bytes of data and stores them at the
> address of v32, stomping on the 4 bytes that follow v32 itself.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> --
> diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
> index 6792928ba84a..155174ddb7ae 100644
> --- a/arch/xtensa/include/asm/uaccess.h
> +++ b/arch/xtensa/include/asm/uaccess.h
> @@ -100,7 +100,7 @@ do {                                                                        \
>         case 4: __put_user_asm(x, ptr, retval, 4, "s32i", __cb); break; \
>         case 8: {                                                       \
>                      __typeof__(*ptr) __v64 = x;                        \
> -                    retval = __copy_to_user(ptr, &__v64, 8);           \
> +                    retval = __copy_to_user(ptr, &__v64, 8) ? -EFAULT : 0;     \

Sure, I agree with that.

>                      break;                                             \
>                 }                                                       \
>         default: __put_user_bad();                                      \
> @@ -198,7 +198,12 @@ do {                                                                       \
>         case 1: __get_user_asm(x, ptr, retval, 1, "l8ui", __cb);  break;\
>         case 2: __get_user_asm(x, ptr, retval, 2, "l16ui", __cb); break;\
>         case 4: __get_user_asm(x, ptr, retval, 4, "l32i", __cb);  break;\
> -       case 8: retval = __copy_from_user(&x, ptr, 8);    break;        \
> +       case 8: {                                                       \
> +               __u64 __x = 0;                                          \
> +               retval = __copy_from_user(&__x, ptr, 8) ? -EFAULT : 0;  \
> +               (x) = *(__force __typeof__(*(ptr)) *) &__x;             \
> +               break;                                                  \
> +       }                                                               \

There's also the following code in the callers of this macro, e.g. in
__get_user_nocheck:

        long __gu_err, __gu_val;                                \
        __get_user_size(__gu_val, (ptr), (size), __gu_err);     \
        (x) = (__force __typeof__(*(ptr)))__gu_val;             \

the last line is important for sizes 1..4, because it takes care of
sign extension of the value loaded by the assembly.
At the same time the first line doesn't make sense for the size 8
as it will result in value truncation.

How about the following change instead:

diff --git a/arch/xtensa/include/asm/uaccess.h
b/arch/xtensa/include/asm/uaccess.h
index 6792928ba84a..c54893651d69 100644
--- a/arch/xtensa/include/asm/uaccess.h
+++ b/arch/xtensa/include/asm/uaccess.h
@@ -100,7 +100,7 @@ do {
                         \
        case 4: __put_user_asm(x, ptr, retval, 4, "s32i", __cb); break; \
        case 8: {                                                       \
                     __typeof__(*ptr) __v64 = x;                        \
-                    retval = __copy_to_user(ptr, &__v64, 8);           \
+                    retval = __copy_to_user(ptr, &__v64, 8) ? -EFAULT
: 0;     \
                     break;                                             \
                }                                                       \
        default: __put_user_bad();                                      \
@@ -172,7 +172,8 @@ __asm__ __volatile__(
         \

 #define __get_user_nocheck(x, ptr, size)                       \
 ({                                                             \
-       long __gu_err, __gu_val;                                \
+       long __gu_err;                                          \
+       __typeof__(*(ptr) + 0) __gu_val;                        \
        __get_user_size(__gu_val, (ptr), (size), __gu_err);     \
        (x) = (__force __typeof__(*(ptr)))__gu_val;             \
        __gu_err;                                               \
@@ -180,7 +181,8 @@ __asm__ __volatile__(
         \

 #define __get_user_check(x, ptr, size)                                 \
 ({                                                                     \
-       long __gu_err = -EFAULT, __gu_val = 0;                          \
+       long __gu_err = -EFAULT;                                        \
+       __typeof__(*(ptr) + 0) __gu_val = 0;                            \
        const __typeof__(*(ptr)) *__gu_addr = (ptr);                    \
        if (access_ok(__gu_addr, size))                 \
                __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
@@ -198,7 +200,7 @@ do {
                         \
        case 1: __get_user_asm(x, ptr, retval, 1, "l8ui", __cb);  break;\
        case 2: __get_user_asm(x, ptr, retval, 2, "l16ui", __cb); break;\
        case 4: __get_user_asm(x, ptr, retval, 4, "l32i", __cb);  break;\
-       case 8: retval = __copy_from_user(&x, ptr, 8);    break;        \
+       case 8: retval = __copy_from_user(&x, ptr, 8) ? -EFAULT : 0;
 break;  \
        default: (x) = __get_user_bad();                                \
        }                                                               \
 } while (0)

Here __typeof__(*(ptr) + 0) makes enough room for all cases
in the __get_user_size and the "+0" part takes care of pointers
to const data.

-- 
Thanks.
-- Max