Re: [PATCH v3 05/46] x86: asm: instrument usercopy in get_user() and __put_user_size()

[Alexander Potapenko]

On Wed, Apr 27, 2022 at 9:15 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Tue, Apr 26, 2022 at 6:42 PM Alexander Potapenko <glider@xxxxxxxxxx> 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.