| Message ID | 20230206-sgx-use-after-iter-v2-1-736ca621adc3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release() | expand |
On Wed, Mar 01, 2023 at 12:17:29PM +0100, Jakob Koschel wrote: > If &encl_mm->encl->mm_list does not contain the searched 'encl_mm', > 'tmp' will not point to a valid sgx_encl_mm struct. > > Linus proposed to avoid any use of the list iterator variable after the > loop, in the attempt to move the list iterator variable declaration into > the marcro to avoid any potential misuse after the loop. > Using it in a pointer comparision after the loop is undefined behavior > and should be omitted if possible [1]. > > Instead we'll just use a 'found' boolean to indicate if an element > was found. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> > --- > Changes in v2: > - refactor to use 'found' variable instead of moving code into the loop > - add cover letter info into the patch > - Link to v1: https://lore.kernel.org/r/20230206-sgx-use-after-iter-v1-1-c09fb5300b5e@gmail.com > --- > arch/x86/kernel/cpu/sgx/encl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 2a0e90fe2abc..91fa70e51004 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -755,6 +755,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, > { > struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); > struct sgx_encl_mm *tmp = NULL; > + bool found = false; > > /* > * The enclave itself can remove encl_mm. Note, objects can't be moved > @@ -764,12 +765,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, > list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) { > if (tmp == encl_mm) { > list_del_rcu(&encl_mm->list); > + found = true; > break; > } > } > spin_unlock(&encl_mm->encl->mm_lock); > > - if (tmp == encl_mm) { > + if (found) { > synchronize_srcu(&encl_mm->encl->srcu); > mmu_notifier_put(mn); > } > > --- > base-commit: d2d11f342b179f1894a901f143ec7c008caba43e > change-id: 20230206-sgx-use-after-iter-f584c1d64c87 > > Best regards, > -- > Jakob Koschel <jkl820.git@gmail.com> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 2a0e90fe2abc..91fa70e51004 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -755,6 +755,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, { struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); struct sgx_encl_mm *tmp = NULL; + bool found = false; /* * The enclave itself can remove encl_mm. Note, objects can't be moved @@ -764,12 +765,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) { if (tmp == encl_mm) { list_del_rcu(&encl_mm->list); + found = true; break; } } spin_unlock(&encl_mm->encl->mm_lock); - if (tmp == encl_mm) { + if (found) { synchronize_srcu(&encl_mm->encl->srcu); mmu_notifier_put(mn); }
If &encl_mm->encl->mm_list does not contain the searched 'encl_mm', 'tmp' will not point to a valid sgx_encl_mm struct. Linus proposed to avoid any use of the list iterator variable after the loop, in the attempt to move the list iterator variable declaration into the marcro to avoid any potential misuse after the loop. Using it in a pointer comparision after the loop is undefined behavior and should be omitted if possible [1]. Instead we'll just use a 'found' boolean to indicate if an element was found. Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> --- Changes in v2: - refactor to use 'found' variable instead of moving code into the loop - add cover letter info into the patch - Link to v1: https://lore.kernel.org/r/20230206-sgx-use-after-iter-v1-1-c09fb5300b5e@gmail.com --- arch/x86/kernel/cpu/sgx/encl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- base-commit: d2d11f342b179f1894a901f143ec7c008caba43e change-id: 20230206-sgx-use-after-iter-f584c1d64c87 Best regards,