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

[Arnd Bergmann]

On Wed, Nov 8, 2017 at 6:54 AM, Greentime Hu <green.hu@xxxxxxxxx> wrote:
> From: Greentime Hu <greentime@xxxxxxxxxxxxx>
>
> Signed-off-by: Vincent Chen <vincentc@xxxxxxxxxxxxx>
> Signed-off-by: Greentime Hu <greentime@xxxxxxxxxxxxx>
>  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