linux-kernel - Re: [PATCH v3 05/46] x86: asm: instrument usercopy in get_user() and __put_user

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] [day] [month] [year] [list]
Message-ID: <CAG_fn=X601D5RtbkOMjZEKL+ZyQZG5Ddw7Uv=MOivbceAxPBAg@mail.gmail.com>
Date:   Thu, 2 Jun 2022 13:20:16 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>, Christoph Hellwig <hch@....de>,
        Christoph Lameter <cl@...ux.com>,
        David Rientjes <rientjes@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Ilya Leoshkevich <iii@...ux.ibm.com>,
        Ingo Molnar <mingo@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Kees Cook <keescook@...omium.org>,
        Marco Elver <elver@...gle.com>,
        Mark Rutland <mark.rutland@....com>,
        Matthew Wilcox <willy@...radead.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Pekka Enberg <penberg@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        kasan-dev <kasan-dev@...glegroups.com>,
        Linux-MM <linux-mm@...ck.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 05/46] x86: asm: instrument usercopy in get_user() and __put_user_size()

On Wed, Apr 27, 2022 at 9:15 AM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Tue, Apr 26, 2022 at 6:42 PM Alexander Potapenko <glider@...gle.com> wrote:
> > @@ -99,11 +100,13 @@ extern int __get_user_bad(void);
> >         int __ret_gu;                                                   \
> >         register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);            \
> >         __chk_user_ptr(ptr);                                            \
> > +       instrument_copy_from_user_before((void *)&(x), ptr, sizeof(*(ptr))); \
> >         asm volatile("call __" #fn "_%P4"                               \
> >                      : "=a" (__ret_gu), "=r" (__val_gu),                \
> >                         ASM_CALL_CONSTRAINT                             \
> >                      : "0" (ptr), "i" (sizeof(*(ptr))));                \
> >         (x) = (__force __typeof__(*(ptr))) __val_gu;                    \
> > +       instrument_copy_from_user_after((void *)&(x), ptr, sizeof(*(ptr)), 0); \
>
> Isn't "ptr" the original pointer here? I think what happened with the
> reported warning is that you get one output line for every instance this
> is used in. There should probably be a
>
>       __auto_type __ptr = (ptr);
>
> at the beginning of the macro to ensure that 'ptr' is only evaluated once.
>
> >>> arch/x86/kernel/signal.c:360:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got unsigned long long [usertype] * @@
>
> It would also make sense to add the missing __user annotation in this line, but
> I suspect there are others like it in drivers.
>
>       Arnd

I ran sparse locally, and it is actually the missing __user
annotations in signal.c that cause these reports.

The following patch:

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index e439eb14325fa..68537dbffa545 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -355,7 +355,7 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
         * reasons and because gdb uses it as a signature to notice
         * signal handler stack frames.
         */
-       unsafe_put_user(*((u64 *)&retcode), (u64 *)frame->retcode, Efault);
+       unsafe_put_user(*((u64 *)&retcode), (__user u64
*)frame->retcode, Efault);
        user_access_end();

        /* Set up registers for signal handler */
@@ -415,7 +415,7 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
         * reasons and because gdb uses it as a signature to notice
         * signal handler stack frames.
         */
-       unsafe_put_user(*((u64 *)&rt_retcode), (u64 *)frame->retcode, Efault);
+       unsafe_put_user(*((u64 *)&rt_retcode), (__user u64
*)frame->retcode, Efault);
        unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set, Efault);
        unsafe_put_sigmask(set, frame, Efault);
        user_access_end();

appears to fix sparse warnings.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.


This e-mail is confidential. If you received this communication by
mistake, please don't forward it to anyone else, please erase all
copies and attachments, and please let me know that it has gone to the
wrong person.