Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault()

[Michael Ellerman]

Christophe LEROY <christophe.leroy@xxxxxx> writes:

> Le 02/06/2017 Ã 11:26, Michael Ellerman a Ãcrit :
>> Christophe Leroy <christophe.leroy@xxxxxx> writes:
>> 
>>> Only the get_user() in store_updates_sp() has to be done outside
>>> the mm semaphore. All the comparison can be done within the semaphore,
>>> so only when really needed.
>>>
>>> As we got a DSI exception, the address pointed by regs->nip is
>>> obviously valid, otherwise we would have had a instruction exception.
>>> So __get_user() can be used instead of get_user()
>> 
>> I don't think that part is true.
>> 
>> You took a DSI so there *was* an instruction at NIP, but since then it
>> may have been unmapped by another thread.
>> 
>> So I don't think you can assume the get_user() will succeed.
>
> The difference between get_user() and __get_user() is that get_user() 
> performs an access_ok() in addition.
>
> Doesn't access_ok() only checks whether addr is below TASK_SIZE to 
> ensure it is a valid user address ?

Yeah more or less, via some gross macros.

I was actually not that worried about the switch from get_user() to
__get_user(), but rather that you removed the check of the return value.
ie. 

-	if (get_user(inst, (unsigned int __user *)regs->nip))
-		return 0;

Became:

	if (is_write && user_mode(regs))
-		store_update_sp = store_updates_sp(regs);
+		__get_user(inst, (unsigned int __user *)regs->nip);


I think dropping the access_ok() probably is alright, because the NIP
must (should!) have been in userspace, though as Ben says it's always
good to be paranoid.

But ignoring that the address can fault at all is wrong AFAICS.

cheers