Re: [RFC PATCH v1 01/10] s390/uaccess: Add storage key checked access to user memory

From: Heiko Carstens <hca@linux.ibm.com>
To: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	Sven Schnelle <svens@linux.ibm.com>,
	Nico Boehr <nrb@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 01/10] s390/uaccess: Add storage key checked access to user memory
Date: Thu, 20 Jan 2022 13:56:40 +0100	[thread overview]
Message-ID: <YelcCEuVbIg9ND90@osiris> (raw)
In-Reply-To: <dbfec527-b995-e382-dafa-c3459e1e45ed@linux.ibm.com>

On Thu, Jan 20, 2022 at 09:34:05AM +0100, Janis Schoetterl-Glausch wrote:
> On 1/19/22 14:20, Heiko Carstens wrote:
> > On Wed, Jan 19, 2022 at 12:02:34PM +0100, Janis Schoetterl-Glausch wrote:
> >>> That's a lot of code churn... I would have expected that the existing
> >>> functions will be renamed, get an additional key parameter, and the
> >>> current API is implemented by defines which map copy_to_user() &
> >>> friends to the new functions, and add a zero key.
> >>
> >> I don't think I understand you. I can implement raw_copy_from/to_user
> >> in terms of raw_copy_from/to_user_with_key, which does save a few lines,
> >> but that's it, isn't it?
> > 
> > Right you are. I only looked at your patch, and forgot about that all
> > the wrapping is nowadays done in common code.
> > 
> > So what I really don't like about this approach is that we get an arch
> > specific copy_to_user() implementation back. This means that all those
> > extra calls like might_fault(), instrument_copy_to_user(), and friends
> > now have to be kept in sync by us again, if new instrumentation or
> > security options are added to common code.
> > 
> > Given that this is manual work / monitoring I'm sure this will not
> > work in the mid or long term, like it has been proven several times in
> > the past for other features. We need something better, which works
> > out-of-the-box wrt common code changes / enhancements.
> 
> What are our options?
> 
> 1. Tooling
>    1.1 Automatic monitoring
>    1.2 ?

No tooling please.

> 2. Implementation changes
>    2.1 Modify common code

In general such changes are done in way that common code is or _may_ be
modified to fulfill our needs. Common code header file explicitely states
that architectures should get rid of private instances of
copy_{to,from}_user() and __copy_{to,from}_user{,_inatomic}().

So we should not add anything like that to arch code again, since nobody
would expect that.

>    2.2 Don't modify common code, pass key argument via well known location

This might also break, and adds complex code and dependencies which should
be avoided.

But maybe you find something else which is nice and easily maintainable.