Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts in do_page_fault() | From | christophe leroy <> | Date | Mon, 5 Jun 2017 19:48:16 +0200 |
| |
Le 05/06/2017 à 12:45, Michael Ellerman a écrit : > Christophe LEROY <christophe.leroy@c-s.fr> writes: > >> Le 02/06/2017 à 11:26, Michael Ellerman a écrit : >>> Christophe Leroy <christophe.leroy@c-s.fr> 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.
Christophe
--- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
| ![\](/images/icornerr.gif) |