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

From: Michael Ellerman
Date: Tue Jun 06 2017 - 07:00:40 EST


christophe leroy <christophe.leroy@xxxxxx> writes:

> Le 05/06/2017 Ã 12:45, Michael Ellerman a Ãcrit :
>> 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.
>
> I see what you mean now.
>
> Indeed,
>
> - unsigned int inst;
>
> Became
>
> + unsigned int inst = 0;
>
> Since __get_user() doesn't modify 'inst' in case of error, 'inst'
> remains 0, and store_updates_sp(0) return false. That was the idea behind.

Ugh. OK, my bad. Though it is a little subtle.

How about:

@@ -286,10 +290,13 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
/*
* We want to do this outside mmap_sem, because reading code around nip
* can result in fault, which will cause a deadlock when called with
- * mmap_sem held
+ * mmap_sem held. We don't need to check if get_user() fails, if it does
+ * it won't modify inst, and an inst of 0 will return false from
+ * store_updates_sp().
*/
+ inst = 0;
if (is_write && is_user)
- store_update_sp = store_updates_sp(regs);
+ get_user(inst, (unsigned int __user *)regs->nip);

if (is_user)
flags |= FAULT_FLAG_USER;


Then this one can go in.

cheers