Re: [RFC PATCH v1 5/6] powerpc/mm: Add a framework for Kernel Userspace Access Protection

[Christophe LEROY <christophe.leroy@c-s.fr>]

Le 22/11/2018 à 02:25, Russell Currey a écrit :
> On Wed, 2018-11-21 at 09:32 +0100, Christophe LEROY wrote:
>>
>> Le 21/11/2018 à 03:26, Russell Currey a écrit :
>>> On Wed, 2018-11-07 at 16:56 +0000, Christophe Leroy wrote:
>>>> This patch implements a framework for Kernel Userspace Access
>>>> Protection.
>>>>
>>>> Then subarches will have to possibility to provide their own
>>>> implementation
>>>> by providing setup_kuap(), and lock/unlock_user_rd/wr_access
>>>>
>>>> We separate read and write accesses because some subarches like
>>>> book3s32 might only support write access protection.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>> Separating read and writes does have a performance impact, I'm
>>> doing
>>> some benchmarking to find out exactly how much - but at least for
>>> radix
>>> it means we have to do a RMW instead of just a write.  It does add
>>> some
>>> amount of security, though.
>>>
>>> The other issue I have is that you're just locking everything here
>>> (like I was), and not doing anything different for just reads or
>>> writes.  In theory, wouldn't someone assume that they could (for
>>> example) unlock reads, lock writes, then attempt to read?  At which
>>> point the read would fail, because the lock actually locks both.
>>>
>>> I would think we either need to bundle read/write locking/unlocking
>>> together, or only implement this on platforms that can do one at a
>>> time, unless there's a cleaner way to handle this.  Glancing at the
>>> values you use for 8xx, this doesn't seem possible there, and it's
>>> a
>>> definite performance hit for radix.
>>>
>>> At the same time, as you say, it would suck for book3s32 that can
>>> only
>>> do writes, but maybe just doing both at the same time and if
>>> implemented for that platform it could just have a warning that it
>>> only
>>> applies to writes on init?
>>
>> Well, I see your points. My idea was not to separate read and write
>> on platform that can lock both. I think it is no problem to also
>> unlocking writes when we are doing a read, so on platforms that can
>> do
>> both I think both should do the same..
>>
>> The idea was to avoid spending time unlocking writes for doing a read
>> on
>> platforms on which reads are not locked. And for platforms able to
>> independently unlock/lock reads and writes, if only unlocking reads
>> can
>> improve performance it can be interesting as well.
>>
>> For book3s/32, locking/unlocking will be done through Kp/Ks bits in
>> segment registers, the function won't be trivial as it may involve
>> more
>> than one segment at a time. So I just wanted to avoid spending time
>> doing that for reads as reads won't be protected. And may also be
>> the
>> case on older book3s/64, may not it ?
>> On Book3s/32, the page protection bits are as follows:
>>
>>     Key	0	1
>> PP
>> 00	RW	NA
>> 01	RW	RO
>> 10	RW	RW
>> 11	RO	RO
>>
>> So the idea is to encode user RW with PP01 (instead of PP10 today)
>> and
>> user RO with PP11 (as done today), giving Key0 to user and Key1 to
>> kernel (today both user and kernel have Key1). Then when kernel needs
>> to
>> write, we change Ks to Key0 in segment register for the involved
>> segments.
>>
>> I'm not sure there is any risk that someone nests unlocks/locks for
>> reads and unlocks/locks for writes, because the unlocks/locks are
>> done
>> in very limited places.
> 
> Yeah I don't think it's a risk since the scope is so limited, it just
> needs to be clearly documented that locking/unlocking reads/writes
> could have the side effect of covering the other.  My concern is less
> about a problem in practice as much as functions that only don't
> exactly do what the function name says.
> 
> Another option is to again have a single lock/unlock function that
> takes a bitmask (so read/write/both), which due to being a singular
> function might be a bit more obvious that it could lock/unlock
> everything, but at this point I'm just bikeshedding.

In order to support book3s/32, I needed to add arguments to the 
unlock/lock functions, as the address is needed to identify the affected 
segments.

Therefore, I changed it to single functions as you suggested. These 
functions have 'to', 'from' and 'size' arguments. When it is a read, 
'to' is NULL. When it is a write, 'from' is NULL. When it is a copy, 
none is NULL.

See RFC v2 for the details.

Christophe

> 
> Doing it this way should be fine, I'm just cautious that some future
> developer might be caught off guard.
> 
> Planning on sending my series based on top of yours for radix today.
> 
> - Russell
> 
>>
>> Christophe
>>
>>
>>> Curious for people's thoughts on this.
>>>
>>> - Russell
>>>