* [PATCH v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release()
@ 2023-03-01 11:17 Jakob Koschel
2023-03-01 23:07 ` Jarkko Sakkinen
2023-06-13 14:39 ` [tip: x86/sgx] " tip-bot2 for Jakob Koschel
0 siblings, 2 replies; 3+ messages in thread
From: Jakob Koschel @ 2023-03-01 11:17 UTC (permalink / raw
To: Jarkko Sakkinen, Dave Hansen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin
Cc: linux-sgx, linux-kernel, Pietro Borrello, Cristiano Giuffrida,
Bos, H.J., Jakob Koschel
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>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release()
2023-03-01 11:17 [PATCH v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release() Jakob Koschel
@ 2023-03-01 23:07 ` Jarkko Sakkinen
2023-06-13 14:39 ` [tip: x86/sgx] " tip-bot2 for Jakob Koschel
1 sibling, 0 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2023-03-01 23:07 UTC (permalink / raw
To: Jakob Koschel
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, linux-sgx, linux-kernel, Pietro Borrello,
Cristiano Giuffrida, Bos, H.J.
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tip: x86/sgx] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release()
2023-03-01 11:17 [PATCH v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release() Jakob Koschel
2023-03-01 23:07 ` Jarkko Sakkinen
@ 2023-06-13 14:39 ` tip-bot2 for Jakob Koschel
1 sibling, 0 replies; 3+ messages in thread
From: tip-bot2 for Jakob Koschel @ 2023-06-13 14:39 UTC (permalink / raw
To: linux-tip-commits
Cc: Jakob Koschel, Borislav Petkov (AMD), Jarkko Sakkinen,
Dave Hansen, x86, linux-kernel
The following commit has been merged into the x86/sgx branch of tip:
Commit-ID: 1e327963cfab0e02eeeb0331178d6c353c959cd6
Gitweb: https://git.kernel.org/tip/1e327963cfab0e02eeeb0331178d6c353c959cd6
Author: Jakob Koschel <jkl820.git@gmail.com>
AuthorDate: Wed, 01 Mar 2023 12:17:29 +01:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 13 Jun 2023 16:21:01 +02:00
x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release()
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 macro to avoid any potential misuse after the loop. Using it in
a pointer comparison after the loop is undefined behavior and should be
omitted if possible, see Link tag.
Instead, just use a 'found' boolean to indicate if an element was found.
[ bp: Massage, fix typos. ]
Signed-off-by: Jakob Koschel <jkl820.git@gmail.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/
Link: https://lore.kernel.org/r/20230206-sgx-use-after-iter-v2-1-736ca621adc3@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 2a0e90f..91fa70e 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);
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-13 14:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-01 11:17 [PATCH v2] x86/sgx: Avoid using iterator after loop in sgx_mmu_notifier_release() Jakob Koschel
2023-03-01 23:07 ` Jarkko Sakkinen
2023-06-13 14:39 ` [tip: x86/sgx] " tip-bot2 for Jakob Koschel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.