[lkml]   [2020]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end

Le 02/04/2020 à 18:29, Al Viro a écrit :
> On Thu, Apr 02, 2020 at 07:34:16AM +0000, Christophe Leroy wrote:
>> Some architectures like powerpc64 have the capability to separate
>> read access and write access protection.
>> For get_user() and copy_from_user(), powerpc64 only open read access.
>> For put_user() and copy_to_user(), powerpc64 only open write access.
>> But when using unsafe_get_user() or unsafe_put_user(),
>> user_access_begin open both read and write.
>> Other architectures like powerpc book3s 32 bits only allow write
>> access protection. And on this architecture protection is an heavy
>> operation as it requires locking/unlocking per segment of 256Mbytes.
>> On those architecture it is therefore desirable to do the unlocking
>> only for write access. (Note that book3s/32 ranges from very old
>> powermac from the 90's with powerpc 601 processor, till modern
>> ADSL boxes with PowerQuicc II modern processors for instance so it
>> is still worth considering)
>> In order to avoid any risk based of hacking some variable parameters
>> passed to user_access_begin/end that would allow hacking and
>> leaving user access open or opening too much, it is preferable to
>> use dedicated static functions that can't be overridden.
>> Add a user_read_access_begin and user_read_access_end to only open
>> read access.
>> Add a user_write_access_begin and user_write_access_end to only open
>> write access.
>> By default, when undefined, those new access helpers default on the
>> existing user_access_begin and user_access_end.
> The only problem I have is that we'd better choose the calling
> conventions that work for other architectures as well.
> AFAICS, aside of ppc and x86 we have (at least) this:
> arm:
> unsigned int __ua_flags = uaccess_save_and_enable();
> ...
> uaccess_restore(__ua_flags);
> arm64:
> uaccess_enable_not_uao();
> ...
> uaccess_disable_not_uao();
> riscv:
> __enable_user_access();
> ...
> __disable_user_access();
> s390/mvc:
> old_fs = enable_sacf_uaccess();
> ...
> disable_sacf_uaccess(old_fs);
> arm64 and riscv are easy - they map well on what we have now.
> The interesting ones are ppc, arm and s390.
> You wants to specify the kind of access; OK, but... it's not just read
> vs. write - there's read-write as well. AFAICS, there are 3 users of
> that:
> * copy_in_user()
> * arch_futex_atomic_op_inuser()
> * futex_atomic_cmpxchg_inatomic()
> The former is of dubious utility (all users outside of arch are in
> the badly done compat code), but the other two are not going to go
> away.

user_access_begin() grants both read and write.

This patch adds user_read_access_begin() and user_write_access_begin()
but it doesn't remove user_access_begin()

> What should we do about that? Do we prohibit such blocks outside
> of arch?
> What should we do about arm and s390? There we want a cookie passed
> from beginning of block to its end; should that be a return value?

That was the way I implemented it in January, see

There was some discussion around that and most noticeable was:

H. Peter (hpa) said about it: "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."

> And at least on arm that thing nests (see e.g. __clear_user_memset()
> there), so "stash that cookie in current->something" is not a solution...
> Folks, let's sort that out while we still have few users of that
> interface; changing the calling conventions later will be much harder.
> Comments?

This patch minimises the change by just adding user_read_access_begin()
and user_write_access_begin() keeping the same parameters as the
existing user_access_begin().

So I can come back to a mix of this patch and the January version if it
corresponds to everyone's view, it will also be a bit easier for powerpc
(especially book3s/32). But that didn't seem to be the expected
direction back when we discussed it in January.


 \ /
  Last update: 2020-04-02 19:04    [W:0.079 / U:3.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site