netdev - Fwd: FW: [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>] [day] [month] [year] [list]

Message-ID: <CAJsyPhzBtbq9boC1pa7objSSc+vnDGKPzGQFKy67i-AzxRwZ3g@mail.gmail.com>
Date:   Mon, 13 Nov 2017 18:54:47 +0800
From:   Vincent Chen <deanbo422@...il.com>
To:     Arnd Bergmann <arndbergmann@...il.com>
Cc:     greentime@...estech.com, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, tglx@...utronix.de,
        jason@...edaemon.net, marc.zyngier@....com, robh+dt@...nel.org,
        netdev@...r.kernel.org
Subject: Fwd: FW: [PATCH 04/31] nds32: Exception handling

>> 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?

Thanks
OK, I will disable alignment fault handling by default in next version patch

>> +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.

Thanks.
I will follow your suggestion to modify it


>> +       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?

Thanks
I will fix it in next version patch.


>> +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.

OK. I will use sysctl instaed of procfs to control this alignment
correction feature.
Thanks

>
>     Arnd

Best regards
Vincent