RE: [PATCH] x86/speculation: Check whether speculation is force disabled
I confirmed that this issue was fixed in the below new patch https://lore.kernel.org/patchwork/patch/1253799/ Thanks. -Original Message- From: Tada, Kenta (Sony) Sent: Friday, June 5, 2020 9:07 PM To: Waiman Long ; x...@kernel.org; t...@linutronix.de; mi...@redhat.com; b...@alien8.de; h...@zytor.com; jpoim...@redhat.com; pet...@infradead.org; tony.l...@intel.com; pawan.kumar.gu...@linux.intel.com Cc: linux-kernel@vger.kernel.org Subject: RE: [PATCH] x86/speculation: Check whether speculation is force disabled I'm sorry but I could not find the reason of above comments. I investigated the below log and I thought it was unintentional and the just bug at the moment. https://lore.kernel.org/lkml/20181125185005.866780...@linutronix.de/ #define PFA_SPEC_IB_FORCE_DISABLE 6 /* Indirect branch speculation permanently restricted */ But the comment of PFA_SPEC_IB_FORCE_DISABLE apparently explains the expected behavior. And it is only natural that users can force disable the speculation because of security. I'll investigate more to explain this patch is needed. Thank you for the review. -Original Message- From: Waiman Long Sent: Friday, June 5, 2020 1:10 AM To: Tada, Kenta (Sony) ; x...@kernel.org; t...@linutronix.de; mi...@redhat.com; b...@alien8.de; h...@zytor.com; jpoim...@redhat.com; pet...@infradead.org; tony.l...@intel.com; pawan.kumar.gu...@linux.intel.com Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86/speculation: Check whether speculation is force disabled On 6/4/20 3:29 AM, Tada, Kenta (Sony) wrote: >> It conflicts with your new code. We can have an argument on whether >> IB should follow how SSB is being handled. Before that is settled, > Thank you for the information. > It conflicts but I think users who read the below document get confused. > Documentation/userspace-api/spec_ctrl.rst. > > Especially, seccomp users must know the difference of this implicit > specification because both IB and SSB are force disabled > simultaneously when seccomp is enabled without SECCOMP_FILTER_FLAG_SPEC_ALLOW > on x86. What I am saying is that you have to make the argument why your patch is the right way to do thing and also make sure that the comment is consistent. Your current patch doesn't do that. Cheers, Longman
RE: [PATCH] x86/speculation: Check whether speculation is force disabled
I'm sorry but I could not find the reason of above comments. I investigated the below log and I thought it was unintentional and the just bug at the moment. https://lore.kernel.org/lkml/20181125185005.866780...@linutronix.de/ #define PFA_SPEC_IB_FORCE_DISABLE 6 /* Indirect branch speculation permanently restricted */ But the comment of PFA_SPEC_IB_FORCE_DISABLE apparently explains the expected behavior. And it is only natural that users can force disable the speculation because of security. I'll investigate more to explain this patch is needed. Thank you for the review. -Original Message- From: Waiman Long Sent: Friday, June 5, 2020 1:10 AM To: Tada, Kenta (Sony) ; x...@kernel.org; t...@linutronix.de; mi...@redhat.com; b...@alien8.de; h...@zytor.com; jpoim...@redhat.com; pet...@infradead.org; tony.l...@intel.com; pawan.kumar.gu...@linux.intel.com Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86/speculation: Check whether speculation is force disabled On 6/4/20 3:29 AM, Tada, Kenta (Sony) wrote: >> It conflicts with your new code. We can have an argument on whether >> IB should follow how SSB is being handled. Before that is settled, > Thank you for the information. > It conflicts but I think users who read the below document get confused. > Documentation/userspace-api/spec_ctrl.rst. > > Especially, seccomp users must know the difference of this implicit > specification because both IB and SSB are force disabled > simultaneously when seccomp is enabled without SECCOMP_FILTER_FLAG_SPEC_ALLOW > on x86. What I am saying is that you have to make the argument why your patch is the right way to do thing and also make sure that the comment is consistent. Your current patch doesn't do that. Cheers, Longman
Re: [PATCH] x86/speculation: Check whether speculation is force disabled
On 6/4/20 3:29 AM, Tada, Kenta (Sony) wrote: It conflicts with your new code. We can have an argument on whether IB should follow how SSB is being handled. Before that is settled, Thank you for the information. It conflicts but I think users who read the below document get confused. Documentation/userspace-api/spec_ctrl.rst. Especially, seccomp users must know the difference of this implicit specification because both IB and SSB are force disabled simultaneously when seccomp is enabled without SECCOMP_FILTER_FLAG_SPEC_ALLOW on x86. What I am saying is that you have to make the argument why your patch is the right way to do thing and also make sure that the comment is consistent. Your current patch doesn't do that. Cheers, Longman
RE: [PATCH] x86/speculation: Check whether speculation is force disabled
>It conflicts with your new code. We can have an argument on whether IB should >follow how SSB is being handled. Before that is settled, Thank you for the information. It conflicts but I think users who read the below document get confused. Documentation/userspace-api/spec_ctrl.rst. Especially, seccomp users must know the difference of this implicit specification because both IB and SSB are force disabled simultaneously when seccomp is enabled without SECCOMP_FILTER_FLAG_SPEC_ALLOW on x86. -Original Message- From: Waiman Long Sent: Thursday, June 4, 2020 12:40 AM To: Tada, Kenta (Sony) ; x...@kernel.org; t...@linutronix.de; mi...@redhat.com; b...@alien8.de; h...@zytor.com; jpoim...@redhat.com; pet...@infradead.org; tony.l...@intel.com; pawan.kumar.gu...@linux.intel.com Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86/speculation: Check whether speculation is force disabled On 6/3/20 3:12 AM, Tada, Kenta (Sony) wrote: > Once PR_SPEC_FORCE_DISABLE is set, users cannot set PR_SPEC_ENABLE. > This commit checks whether PR_SPEC_FORCE_DISABLE was previously set. > > Signed-off-by: Kenta Tada > --- > arch/x86/kernel/cpu/bugs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index ed54b3b21c39..678ace157035 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -1173,6 +1173,9 @@ static int ib_prctl_set(struct task_struct *task, > unsigned long ctrl) > if (spectre_v2_user == SPECTRE_V2_USER_STRICT || > spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED) > return -EPERM; > + /* If speculation is force disabled, enable is not allowed */ > + if (task_spec_ib_force_disable(task)) > + return -EPERM; > task_clear_spec_ib_disable(task); > task_update_spec_tif(task); > break; There is a comment up a few lines about this: /* * Indirect branch speculation is always allowed when * mitigation is force disabled. */ It conflicts with your new code. We can have an argument on whether IB should follow how SSB is being handled. Before that is settled, Nacked-by: Waiman Long
Re: [PATCH] x86/speculation: Check whether speculation is force disabled
On 6/3/20 3:12 AM, Tada, Kenta (Sony) wrote: Once PR_SPEC_FORCE_DISABLE is set, users cannot set PR_SPEC_ENABLE. This commit checks whether PR_SPEC_FORCE_DISABLE was previously set. Signed-off-by: Kenta Tada --- arch/x86/kernel/cpu/bugs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index ed54b3b21c39..678ace157035 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1173,6 +1173,9 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) if (spectre_v2_user == SPECTRE_V2_USER_STRICT || spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED) return -EPERM; + /* If speculation is force disabled, enable is not allowed */ + if (task_spec_ib_force_disable(task)) + return -EPERM; task_clear_spec_ib_disable(task); task_update_spec_tif(task); break; There is a comment up a few lines about this: /* * Indirect branch speculation is always allowed when * mitigation is force disabled. */ It conflicts with your new code. We can have an argument on whether IB should follow how SSB is being handled. Before that is settled, Nacked-by: Waiman Long
[PATCH] x86/speculation: Check whether speculation is force disabled
Once PR_SPEC_FORCE_DISABLE is set, users cannot set PR_SPEC_ENABLE. This commit checks whether PR_SPEC_FORCE_DISABLE was previously set. Signed-off-by: Kenta Tada --- arch/x86/kernel/cpu/bugs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index ed54b3b21c39..678ace157035 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1173,6 +1173,9 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) if (spectre_v2_user == SPECTRE_V2_USER_STRICT || spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED) return -EPERM; + /* If speculation is force disabled, enable is not allowed */ + if (task_spec_ib_force_disable(task)) + return -EPERM; task_clear_spec_ib_disable(task); task_update_spec_tif(task); break; -- 2.20.1
