From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751362AbdKHIXS (ORCPT ); Wed, 8 Nov 2017 03:23:18 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:55546 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbdKHIXQ (ORCPT ); Wed, 8 Nov 2017 03:23:16 -0500 X-Google-Smtp-Source: ABhQp+ReDcXvI694hFNJaAsgLitSjiRTiAlWs9I517EtM+sBmmyjYxhHUhxT8r0lXV/4RJYCDPnKD0Fur5dspinW+BU= MIME-Version: 1.0 In-Reply-To: <9caaf73b79b22357ef957382eb7c0151a2d7d189.1510118606.git.green.hu@gmail.com> References: <9caaf73b79b22357ef957382eb7c0151a2d7d189.1510118606.git.green.hu@gmail.com> From: Arnd Bergmann Date: Wed, 8 Nov 2017 09:23:15 +0100 X-Google-Sender-Auth: mH3wy01E_9xOKS_RUs3MJqeaCnA Message-ID: Subject: Re: [PATCH 04/31] nds32: Exception handling To: Greentime Hu Cc: greentime@andestech.com, Linux Kernel Mailing List , linux-arch , Thomas Gleixner , Jason Cooper , Marc Zyngier , Rob Herring , Networking , Vincent Chen Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 8, 2017 at 6:54 AM, Greentime Hu wrote: > From: Greentime Hu > > Signed-off-by: Vincent Chen > Signed-off-by: Greentime Hu > 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