netdev - Re: [PATCH 04/31] nds32: Exception handling

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]

Date:   Wed, 8 Nov 2017 09:23:15 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Greentime Hu <green.hu@...il.com>
Cc:     greentime@...estech.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <marc.zyngier@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        Vincent Chen <vincentc@...estech.com>
Subject: Re: [PATCH 04/31] nds32: Exception handling

On Wed, Nov 8, 2017 at 6:54 AM, Greentime Hu <green.hu@...il.com> wrote:
> From: Greentime Hu <greentime@...estech.com>
>
> Signed-off-by: Vincent Chen <vincentc@...estech.com>
> Signed-off-by: Greentime Hu <greentime@...estech.com>
>  arch/nds32/mm/alignment.c       |  564 +++++++++++++++++++++++++++++++++++++++

> diff --git a/arch/nds32/mm/alignment.c b/arch/nds32/mm/alignment.c
> new file mode 100644
> index 0000000..05589e7
> --- /dev/null
> +++ b/arch/nds32/mm/alignment.c

> +static int mode = 0x3;
> +module_param(mode, int, S_IWUSR | S_IRUGO);

It's an interesting question how to best handle alignment faults, both in
kernel and user mode. While it can help for debugging to have the handler,
I'd argue that you are better off in the long run not fixing up the faults
automatically but to modify the code that triggers them instead.

How about making the faults disabled by default?

> +static int _do_unaligned_access(unsigned long entry, unsigned long addr,
> +                               unsigned long type, struct pt_regs *regs)
> +{
> +       unsigned long inst;
> +       int ret = -EFAULT;
> +
> +       if (user_mode(regs)) {
> +               /* user mode */
> +               if (!va_present(current->mm, addr))
> +                       return ret;
> +       } else {
> +               /* kernel mode */
> +               if (!va_kernel_present(addr))
> +                       return ret;
> +       }

This looks racy, the address might be present when you get here, but not
later when you actually access it. I think what you need here is something
like ARM does with get32_unaligned_check() etc and their fixup tables.

> +       inst = get_inst(regs->ipc);
> +
> +       DEBUG(mode & 0x04, 1,
> +             "Faulting Addr: 0x%08lx, PC: 0x%08lx [ 0x%08lx ]\n", addr,
> +             regs->ipc, inst);
> +
> +       if ((user_mode(regs) && (mode & 0x01))
> +           || (!user_mode(regs) && (mode & 0x02))) {
> +
> +               mm_segment_t seg = get_fs();
> +
> +               set_fs(KERNEL_DS);
> +
> +               if (inst & 0x80000000)
> +                       ret = do_16((inst >> 16) & 0xffff, regs);
> +               else
> +                       ret = do_32(inst, regs);
> +
> +               set_fs(seg);
> +       }
> +
> +       return ret;
> +}

Doesn't this allow user space to read all of kernel memory simply by
passing unaligned addresses?

> +static const struct file_operations fops = {
> +       .open = simple_open,
> +       .read = proc_alignment_read,
> +       .write = proc_alignment_write,
> +};

This should really be a sysctl rather than an open-coded procfs file,
for consistency with other architectures.

Please have a look at that interface on other architectures and pick
whatever the majority do.

     Arnd