From: Catalin Marinas <catalin.marinas@arm•com>
To: Al Viro <viro@zeniv•linux.org.uk>
Cc: linux-arch@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
Kees Cook <keescook@chromium•org>,
Christian Borntraeger <borntraeger@de•ibm.com>,
airlied@linux•ie, hpa@zytor•com, linux-kernel@vger•kernel.org,
Russell King <linux@armlinux•org.uk>,
linux-mm@kvack•org, Paul Mackerras <paulus@samba•org>,
daniel@ffwll•ch, akpm@linux-foundation•org,
torvalds@linux-foundation•org
Subject: Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
Date: Fri, 3 Apr 2020 12:26:10 +0100 [thread overview]
Message-ID: <20200403112609.GB26633@mbp> (raw)
In-Reply-To: <20200403005831.GI23230@ZenIV.linux.org.uk>
On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote:
> On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
> > Yup, I think it's a weakness of the ARM implementation and I'd like to
> > not extend it further. AFAIK we should never nest, but I would not be
> > surprised at all if we did.
> >
> > If we were looking at a design goal for all architectures, I'd like
> > to be doing what the public PaX patchset did for their memory access
> > switching, which is to alarm if calling into "enable" found the access
> > already enabled, etc. Such a condition would show an unexpected nesting
> > (like we've seen with similar constructs with set_fs() not getting reset
> > during an exception handler, etc etc).
>
> FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like
> KERNEL_DS is somewhat more dangerous there than on e.g. x86.
>
> Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore
> per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address
> ranges), allowing them even if they would normally be denied. We need
> that for actual uaccess loads/stores, since those use insns that pretend
> to be done in user mode and we want them to access the kernel pages.
> But that affects the normal loads/stores as well; unless I'm misreading
> that code, it will ignore (supervisor) r/o on a page. And that's not
> just for the code inside the uaccess blocks; *everything* done under
> KERNEL_DS is subject to that.
That's correct. Luckily this only affects ARMv5 and earlier. From ARMv6
onwards, CONFIG_CPU_USE_DOMAINS is no longer selected and the uaccess
instructions are just plain ldr/str.
Russell should know the details on whether there was much choice. Since
the kernel was living in the linear map with full rwx permissions, the
KERNEL_DS overriding was probably not a concern and the ldrt/strt for
uaccess deemed more secure. We also have weird permission setting
pre-ARMv6 (or rather v6k) where RO user pages are writable from the
kernel with standard str instructions (breaking CoW). I don't recall
whether it was a choice made by the kernel or something the architecture
enforced. The vectors page has to be kernel writable (and user RO) to
store the TLS value in the absence of a TLS register but maybe we could
do this via the linear alias together with the appropriate cache
maintenance.
From ARMv6, the domain overriding had the side-effect of ignoring the XN
bit and causing random instruction fetches from ioremap() areas. So we
had to remove the domain switching. We also gained a dedicated TLS
register.
> Why do we do that (modify_domain(), that is) inside set_fs() and not
> in uaccess_enable() et.al.?
I think uaccess_enable() could indeed switch the kernel domain if
KERNEL_DS is set and move this out of set_fs(). It would reduce the
window the kernel domain permissions are overridden. Anyway,
uaccess_enable() appeared much later on arm when Russell introduced PAN
(SMAP) like support by switching the user domain.
--
Catalin
next prev parent reply other threads:[~2020-04-03 11:30 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-02 7:34 [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Christophe Leroy
2020-04-02 7:34 ` [PATCH RESEND 2/4] uaccess: Selectively open read or write user access Christophe Leroy
2020-04-02 7:51 ` Kees Cook
2020-04-02 8:00 ` Christophe Leroy
2020-04-02 7:34 ` [PATCH RESEND 3/4] drm/i915/gem: Replace user_access_begin by user_write_access_begin Christophe Leroy
2020-04-02 7:52 ` Kees Cook
2020-04-02 7:59 ` Christophe Leroy
2020-04-02 7:34 ` [PATCH RESEND 4/4] powerpc/uaccess: Implement user_read_access_begin and user_write_access_begin Christophe Leroy
2020-04-02 7:52 ` Kees Cook
2020-04-02 7:46 ` [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Kees Cook
2020-04-02 16:29 ` Al Viro
2020-04-02 17:03 ` Christophe Leroy
2020-04-02 17:38 ` Kees Cook
2020-04-02 17:50 ` Al Viro
2020-04-02 18:35 ` Christophe Leroy
2020-04-02 18:35 ` Kees Cook
2020-04-02 19:26 ` Linus Torvalds
2020-04-02 20:27 ` Kees Cook
2020-04-02 20:47 ` Linus Torvalds
2020-04-03 0:58 ` Al Viro
2020-04-03 9:49 ` Russell King - ARM Linux admin
2020-04-03 11:26 ` Catalin Marinas [this message]
2020-04-03 13:37 ` Russell King - ARM Linux admin
2020-04-03 17:26 ` Al Viro
2020-04-03 10:02 ` Russell King - ARM Linux admin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200403112609.GB26633@mbp \
--to=catalin.marinas@arm$(echo .)com \
--cc=airlied@linux$(echo .)ie \
--cc=akpm@linux-foundation$(echo .)org \
--cc=borntraeger@de$(echo .)ibm.com \
--cc=daniel@ffwll$(echo .)ch \
--cc=hpa@zytor$(echo .)com \
--cc=keescook@chromium$(echo .)org \
--cc=linux-arch@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-mm@kvack$(echo .)org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=paulus@samba$(echo .)org \
--cc=torvalds@linux-foundation$(echo .)org \
--cc=viro@zeniv$(echo .)linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox