Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 23 Jan 2020 18:03:14 -0800
User-Agent: K-9 Mail for Android
In-Reply-To: <CAHk-=wi8FvaeRv6PpisQ+L_Cv52yE6jCxZzUHQPZ_K7HzFkaBQ@mail.gmail.com>
References: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr> <e11a8f0670251267f87e3114e0bdbacb1eb72980.1579783936.git.christophe.leroy@c-s.fr> <CAHk-=wg4HEABOZdjxMzbembNmxs1zYfrNAEc2L+JS9FBSnM8JA@mail.gmail.com> <fc5c94a2-5a25-0715-5240-5ba3cbe0f2b2@c-s.fr> <CAHk-=wi8FvaeRv6PpisQ+L_Cv52yE6jCxZzUHQPZ_K7HzFkaBQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 8BIT
Subject: Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        christophe leroy <christophe.leroy@c-s.fr>
CC:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        the arch/x86 maintainers <x86@kernel.org>
From:   hpa@zytor.com
Message-ID: <1BC4F810-1BF4-4C15-9113-890A163FDBE2@zytor.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On January 23, 2020 11:57:57 AM PST, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Thu, Jan 23, 2020 at 11:47 AM christophe leroy
><christophe.leroy@c-s.fr> wrote:
>>
>> I'm going to leave it aside, at least for the time being, and do it
>as a
>> second step later after evaluating the real performance impact. I'll
>> respin tomorrow in that way.
>
>Ok, good.
>
>From a "narrow the access window type" standpoint it does seem to be a
>good idea to specify what kind of user accesses will be done, so I
>don't hate the idea, it's more that I'm not convinced it matters
>enough.
>
>On x86, we have made the rule that user_access_begin/end() can contain
>_very_ few operations, and objtool really does enforce that. With
>objtool and KASAN, you really end up with very small ranges of
>user_access_begin/end().
>
>And since we actually verify it statically on x86-64, I would say that
>the added benefit of narrowing by access type is fairly small. We're
>not going to have complicated code in that user access region, at
>least in generic code.
>
>> > Also, it shouldn't be a "is this a write". What if it's a read
>_and_ a
>> > write? Only a write? Only a read?
>>
>> Indeed that was more: does it includes a write. It's either RO or RW
>
>I would expect that most actual users would be RO or WO, so it's a bit
>odd to have those choices.
>
>Of course, often writing ends up requiring read permissions anyway if
>the architecture has problems with alignment handling or similar, but
>still... The real RW case does exist conceptually (we have
>"copy_in_user()", after all), but still feels like it shouldn't be
>seen as the only _interface_ choice.
>
>IOW, an architecture may decide to turn WO into RW because of
>architecture limitations (or, like x86 and arm, ignore the whole
>RO/RW/WO _entirely_ because there's just a single "allow user space
>accesses" flag), but on an interface layer if we add this flag, I
>really think it should be an explicit "read or write or both".
>
>So thus my "let's try to avoid doing it in the first place, but if we
>_do_ do this, then do it right" plea.
>
>                 Linus

I'm wondering if we should make it a static part of the API instead of a variable.

I have *deep* concern with carrying state in a "key" variable: it's a direct attack vector for a crowbar attack, especially since it is by definition live inside a user access region.

One major reason x86 restricts the regions like this is to minimize the amount of unconstrained state: we don't save and restore the state around, but enter and exit unconditionally, which means that a leaked state will end up having a limited lifespan. Nor is there any state inside the user access region which could be corrupted to leave the region open.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.