lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Oct 2020 16:15:40 -0300 From: Jason Gunthorpe <jgg@...dia.com> To: Jann Horn <jannh@...gle.com> CC: Andrew Morton <akpm@...ux-foundation.org>, <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>, "Eric W . Biederman" <ebiederm@...ssion.com>, Michel Lespinasse <walken@...gle.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, Jeff Dike <jdike@...toit.com>, Richard Weinberger <richard@....at>, Anton Ivanov <anton.ivanov@...bridgegreys.com>, <linux-um@...ts.infradead.org>, "John Hubbard" <jhubbard@...dia.com>, Johannes Berg <johannes@...solutions.net> Subject: Re: [PATCH resend v3 2/2] exec: Broadly lock nascent mm until setup_arg_pages() On Sat, Oct 17, 2020 at 12:57:13AM +0200, Jann Horn wrote: > @@ -374,17 +366,12 @@ static int bprm_mm_init(struct linux_binprm *bprm) > task_unlock(current->group_leader); > > err = __bprm_mm_init(bprm); > - if (err) > - goto err; > - > - return 0; > - > -err: > - if (mm) { > - bprm->mm = NULL; > - mmdrop(mm); > - } > + if (!err) > + return 0; > > + bprm->mm = NULL; > + mmap_write_unlock(mm); > + mmdrop(mm); > return err; nit, but prefer 'success-oriented-flow' eg invert the 'if (!err)' and put the error unwind in the {} > @@ -1545,6 +1532,18 @@ void setup_new_exec(struct linux_binprm * bprm) > me->mm->task_size = TASK_SIZE; > mutex_unlock(&me->signal->exec_update_mutex); > mutex_unlock(&me->signal->cred_guard_mutex); > + > + if (!IS_ENABLED(CONFIG_MMU)) { > + /* > + * On MMU, setup_arg_pages() wants to access bprm->vma after > + * this point, so we can't drop the mmap lock yet. > + * On !MMU, we have neither setup_arg_pages() nor bprm->vma, > + * so we should drop the lock here. > + */ > + mmap_write_unlock(bprm->mm); > + mmput(bprm->mm); > + bprm->mm = NULL; > + } The only thing I dislike about this is how tricky the lock lifetime is, it all looks correct, but expecting the setup_arg_pages() or setup_new_exec() to unlock (depending!) is quite tricky. It feels like it would be clearer to have an explicit function to do this, like 'release_brp_mm()' indicating that current->mm is now the only way to get the mm and it must be locked. Or, more practically, the load_binary functionc can now call vm_mmap(). Anyhow, it took a bit to study all the parts but I think it looks right as is. Jason
Powered by blists - more mailing lists