linux-kernel - Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set

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]

Message-ID: <CAK8P3a22EiD-uMZQaBpHQYyy=MJ_7J-ih=6CtgH_9RXT6OOYvg@mail.gmail.com>
Date:   Thu, 17 Sep 2020 19:29:37 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christoph Hellwig <hch@....de>
Cc:     Russell King <rmk@....linux.org.uk>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux- <kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set_fs()

On Tue, Sep 8, 2020 at 8:15 AM Christoph Hellwig <hch@....de> wrote:
>
> > +static void dump_mem(const char *, const char *, unsigned long, unsigned long, bool kernel_mode);
>
> This adds a pointlessly long line.

Fixed.

> And looking at the code I don't see why the argument is even needed.
>
> dump_mem() currently does an unconditional set_fs(KERNEL_DS), so it
> should always use get_kernel_nofault.

I had looked at

        if (!user_mode(regs) || in_interrupt()) {
                dump_mem(KERN_EMERG, "Stack: ", regs->ARM_sp,
                         THREAD_SIZE + (unsigned
long)task_stack_page(tsk), kernel_mode);

which told me that there should be at least some code path ending up in
__die() that has in_interrupt() set but comes from user mode.

In this case, the memory to print would be a user pointer and cannot be
accessed by get_kernel_nofault() (but could be accessed with
"set_fs(KERNEL_DS); __get_user()").

I looked through the history now and the only code path I could
find that would arrive here this way is from bad_mode(), indicating
that there is probably a hardware bug or the contents of *regs are
corrupted.

Russell might have a better explanation for this, but I would assume
now that you are right in that we don't ever need to care about
dumping user space addresses here.

> > +static void dump_instr(const char *lvl, struct pt_regs *regs)
> >  {
> >       unsigned long addr = instruction_pointer(regs);
> >       const int thumb = thumb_mode(regs);
> > @@ -173,10 +169,20 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
> >       for (i = -4; i < 1 + !!thumb; i++) {
> >               unsigned int val, bad;
> >
> > -             if (thumb)
> > -                     bad = get_user(val, &((u16 *)addr)[i]);
> > -             else
> > -                     bad = get_user(val, &((u32 *)addr)[i]);
> > +             if (!user_mode(regs)) {
> > +                     if (thumb) {
> > +                             u16 val16;
> > +                             bad = get_kernel_nofault(val16, &((u16 *)addr)[i]);
> > +                             val = val16;
> > +                     } else {
> > +                             bad = get_kernel_nofault(val, &((u32 *)addr)[i]);
> > +                     }
> > +             } else {
> > +                     if (thumb)
> > +                             bad = get_user(val, &((u16 *)addr)[i]);
> > +                     else
> > +                             bad = get_user(val, &((u32 *)addr)[i]);
> > +             }
>
> When I looked at this earlier I just added a little helper to make
> this a little easier to read.   Here is my patch from an old tree:
>
> http://git.infradead.org/users/hch/misc.git/commitdiff/67413030ccb7a64a7eb828e13ff0795f4eadfeb7

I think your version was broken for the in-kernel thumb version
because get_kernel_nofault does not widen the result
from 16 to 32 bits. I tried fixing this in your version, but the
result ended up more complex than my version here, so I
decided to keep what I had.

       Arnd