linux-kernel - Re: [PATCH 1/6] syscalls: use uaccess_kernel in addr_limit_user

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: <20200721052022.GA10011@lst.de>
Date:   Tue, 21 Jul 2020 07:20:22 +0200
From:   Christoph Hellwig <hch@....de>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Christoph Hellwig <hch@....de>, Nick Hu <nickhu@...estech.com>,
        Greentime Hu <green.hu@...il.com>,
        Vincent Chen <deanbo422@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-riscv@...ts.infradead.org, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] syscalls: use uaccess_kernel in
 addr_limit_user_check

On Mon, Jul 20, 2020 at 10:15:37PM -0700, Guenter Roeck wrote:
> >> -       if (CHECK_DATA_CORRUPTION(uaccess_kernel(),
> >> +       if (CHECK_DATA_CORRUPTION(!uaccess_kernel(),
> >>
> >> How does this work anywhere ?
> > 
> > No, that is the wrong check - we want to make sure the address
> > space override doesn't leak to userspace.  The problem is that
> > armnommu (and m68knommu, but that doesn't call the offending
> > function) pretends to not have a kernel address space, which doesn't
> > really work.  Here is the fix I sent out yesterday, which I should
> > have Cc'ed you on, sorry:
> > 
> 
> The patch below makes sense, and it does work, but I still suspect
> that something with your original patch is wrong, or at least suspicious.
> Reason: My change above (Adding the "!") works for _all_ of my arm boot
> tests. Or, in other words, it doesn't make a difference if true
> or false is passed as first parameter of CHECK_DATA_CORRUPTION(), except
> for nommu systems. Also, unless I am really missing something, your
> original patch _does_ reverse the logic.

Well.  segment_eq is in current mainline used in two places:

 1) to implement uaccess_kernel
 2) in addr_limit_user_check to implement uaccess_kernel-like
    semantics using a strange reverse notation

I think the explanation for your observation is how addr_limit_user_check
is called on arm.  The addr_limit_check_failed wrapper for it is called
from assembly code, but only after already checking the addr_limit,
basically duplicating the segment_eq check.  So for mmu builds it won't
get called unless we leak the kernel address space override, which
is a pretty fatal error and won't show up in your boot tests.  The
only good way to test it is by explicit injecting it using the
lkdtm module.