| Message ID | 151703977742.26578.8362387033092864423.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Dan Williams <dan.j.williams@intel.com> wrote: > Reflect the presence of 'get_user', '__get_user', and 'syscall' > protections in sysfs. Keep the "Vulnerable" distinction given the > expectation that the places that have been identified for 'array_idx' > usage are likely incomplete. (The style problems/inconsistencies of the previous patches are repeated here too, please fix.) > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: x86@kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Reported-by: Jiri Slaby <jslaby@suse.cz> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > arch/x86/kernel/cpu/bugs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index 390b3dc3d438..01d5ba48f745 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -269,7 +269,7 @@ ssize_t cpu_show_spectre_v1(struct device *dev, > { > if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1)) > return sprintf(buf, "Not affected\n"); > - return sprintf(buf, "Vulnerable\n"); > + return sprintf(buf, "Vulnerable: Minimal user pointer sanitization\n"); Btw., I think this string is still somewhat passive-aggressive towards users, as it doesn't really give them any idea about what is missing from their system so that they can turn it into not vulnerable. What else is missing that would turn this into a "Mitigated" entry? Thanks, Ingo
On Sun, Jan 28, 2018 at 1:50 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Dan Williams <dan.j.williams@intel.com> wrote: > >> Reflect the presence of 'get_user', '__get_user', and 'syscall' >> protections in sysfs. Keep the "Vulnerable" distinction given the >> expectation that the places that have been identified for 'array_idx' >> usage are likely incomplete. > > (The style problems/inconsistencies of the previous patches are repeated here too, > please fix.) > >> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: "H. Peter Anvin" <hpa@zytor.com> >> Cc: x86@kernel.org >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Reported-by: Jiri Slaby <jslaby@suse.cz> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> arch/x86/kernel/cpu/bugs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c >> index 390b3dc3d438..01d5ba48f745 100644 >> --- a/arch/x86/kernel/cpu/bugs.c >> +++ b/arch/x86/kernel/cpu/bugs.c >> @@ -269,7 +269,7 @@ ssize_t cpu_show_spectre_v1(struct device *dev, >> { >> if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1)) >> return sprintf(buf, "Not affected\n"); >> - return sprintf(buf, "Vulnerable\n"); >> + return sprintf(buf, "Vulnerable: Minimal user pointer sanitization\n"); > > Btw., I think this string is still somewhat passive-aggressive towards users, as > it doesn't really give them any idea about what is missing from their system so > that they can turn it into not vulnerable. > > What else is missing that would turn this into a "Mitigated" entry? Part of the problem is that there are different sub-classes of Spectre variant1 vulnerabilities. For example, speculating on the value of a user pointer returned from get_user() is mitigated by these kernel changes. However, cleaning up occasions where the CPU might speculate on the validity of a user-controlled pointer offset, or user-controlled array index is only covered by manual inspection of some noisy / incomplete tooling results. I.e. the handful of array_index_nospec() usages in this series is likely incomplete. The usage of barrier_nospec() in __get_user() and open coded array_index_nospec() in get_user() does raise the bar and mitigates an entire class of problems. Perhaps it would be reasonable to have cpu_show_spectre_v1() emit: "Mitigation: __user pointer sanitization" ...with the expectation that the kernel community intends to use new and better tooling to find more places to use array_index_nospec(). Once there is wider confidence in that tooling, or a compiler that does it automatically, the kernel can emit: "Mitigation: automated user input sanitization" ...or something like that.
* Dan Williams <dan.j.williams@intel.com> wrote: > On Sun, Jan 28, 2018 at 1:50 AM, Ingo Molnar <mingo@kernel.org> wrote: > > > > * Dan Williams <dan.j.williams@intel.com> wrote: > > > >> Reflect the presence of 'get_user', '__get_user', and 'syscall' > >> protections in sysfs. Keep the "Vulnerable" distinction given the > >> expectation that the places that have been identified for 'array_idx' > >> usage are likely incomplete. > > > > (The style problems/inconsistencies of the previous patches are repeated here too, > > please fix.) > > > >> > >> Cc: Thomas Gleixner <tglx@linutronix.de> > >> Cc: Ingo Molnar <mingo@redhat.com> > >> Cc: "H. Peter Anvin" <hpa@zytor.com> > >> Cc: x86@kernel.org > >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> Reported-by: Jiri Slaby <jslaby@suse.cz> > >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > >> --- > >> arch/x86/kernel/cpu/bugs.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > >> index 390b3dc3d438..01d5ba48f745 100644 > >> --- a/arch/x86/kernel/cpu/bugs.c > >> +++ b/arch/x86/kernel/cpu/bugs.c > >> @@ -269,7 +269,7 @@ ssize_t cpu_show_spectre_v1(struct device *dev, > >> { > >> if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1)) > >> return sprintf(buf, "Not affected\n"); > >> - return sprintf(buf, "Vulnerable\n"); > >> + return sprintf(buf, "Vulnerable: Minimal user pointer sanitization\n"); > > > > Btw., I think this string is still somewhat passive-aggressive towards users, as > > it doesn't really give them any idea about what is missing from their system so > > that they can turn it into not vulnerable. > > > > What else is missing that would turn this into a "Mitigated" entry? > > Part of the problem is that there are different sub-classes of Spectre > variant1 vulnerabilities. For example, speculating on the value of a > user pointer returned from get_user() is mitigated by these kernel > changes. However, cleaning up occasions where the CPU might speculate > on the validity of a user-controlled pointer offset, or > user-controlled array index is only covered by manual inspection of > some noisy / incomplete tooling results. I.e. the handful of > array_index_nospec() usages in this series is likely incomplete. > > The usage of barrier_nospec() in __get_user() and open coded > array_index_nospec() in get_user() does raise the bar and mitigates an > entire class of problems. Perhaps it would be reasonable to have > cpu_show_spectre_v1() emit: > > "Mitigation: __user pointer sanitization" Yeah, so I think the ideal approach would be if we emitted _both_ pieces of information: Mitigation: __user pointer sanitization Vulnerable: incomplete array index sanitization I.e. we inform the user about the mitigation measures that are active, but we also fully inform that we think it's incomplete at this stage. > ...with the expectation that the kernel community intends to use new > and better tooling to find more places to use array_index_nospec(). > Once there is wider confidence in that tooling, or a compiler that > does it automatically, the kernel can emit: > > "Mitigation: automated user input sanitization" > > ...or something like that. Yeah. Thanks, Ingo
On Wed, Jan 31, 2018 at 12:07 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Dan Williams <dan.j.williams@intel.com> wrote: > >> On Sun, Jan 28, 2018 at 1:50 AM, Ingo Molnar <mingo@kernel.org> wrote: >> > >> > * Dan Williams <dan.j.williams@intel.com> wrote: >> > >> >> Reflect the presence of 'get_user', '__get_user', and 'syscall' >> >> protections in sysfs. Keep the "Vulnerable" distinction given the >> >> expectation that the places that have been identified for 'array_idx' >> >> usage are likely incomplete. >> > >> > (The style problems/inconsistencies of the previous patches are repeated here too, >> > please fix.) >> > >> >> >> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> >> Cc: Ingo Molnar <mingo@redhat.com> >> >> Cc: "H. Peter Anvin" <hpa@zytor.com> >> >> Cc: x86@kernel.org >> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> Reported-by: Jiri Slaby <jslaby@suse.cz> >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> >> --- >> >> arch/x86/kernel/cpu/bugs.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c >> >> index 390b3dc3d438..01d5ba48f745 100644 >> >> --- a/arch/x86/kernel/cpu/bugs.c >> >> +++ b/arch/x86/kernel/cpu/bugs.c >> >> @@ -269,7 +269,7 @@ ssize_t cpu_show_spectre_v1(struct device *dev, >> >> { >> >> if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1)) >> >> return sprintf(buf, "Not affected\n"); >> >> - return sprintf(buf, "Vulnerable\n"); >> >> + return sprintf(buf, "Vulnerable: Minimal user pointer sanitization\n"); >> > >> > Btw., I think this string is still somewhat passive-aggressive towards users, as >> > it doesn't really give them any idea about what is missing from their system so >> > that they can turn it into not vulnerable. >> > >> > What else is missing that would turn this into a "Mitigated" entry? >> >> Part of the problem is that there are different sub-classes of Spectre >> variant1 vulnerabilities. For example, speculating on the value of a >> user pointer returned from get_user() is mitigated by these kernel >> changes. However, cleaning up occasions where the CPU might speculate >> on the validity of a user-controlled pointer offset, or >> user-controlled array index is only covered by manual inspection of >> some noisy / incomplete tooling results. I.e. the handful of >> array_index_nospec() usages in this series is likely incomplete. >> >> The usage of barrier_nospec() in __get_user() and open coded >> array_index_nospec() in get_user() does raise the bar and mitigates an >> entire class of problems. Perhaps it would be reasonable to have >> cpu_show_spectre_v1() emit: >> >> "Mitigation: __user pointer sanitization" > > Yeah, so I think the ideal approach would be if we emitted _both_ pieces of > information: > > Mitigation: __user pointer sanitization > Vulnerable: incomplete array index sanitization > > I.e. we inform the user about the mitigation measures that are active, but we also > fully inform that we think it's incomplete at this stage. But that would assume that it's only array index sanitization that we need to worry about. The get_user() protections were interesting because they showed a class of potential leaks near pointer de-references not necessarily arrays. So I think it's more likely the case that we'll add more "Mitigation:" lines over time.
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 390b3dc3d438..01d5ba48f745 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -269,7 +269,7 @@ ssize_t cpu_show_spectre_v1(struct device *dev, { if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1)) return sprintf(buf, "Not affected\n"); - return sprintf(buf, "Vulnerable\n"); + return sprintf(buf, "Vulnerable: Minimal user pointer sanitization\n"); } ssize_t cpu_show_spectre_v2(struct device *dev,
Reflect the presence of 'get_user', '__get_user', and 'syscall' protections in sysfs. Keep the "Vulnerable" distinction given the expectation that the places that have been identified for 'array_idx' usage are likely incomplete. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reported-by: Jiri Slaby <jslaby@suse.cz> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- arch/x86/kernel/cpu/bugs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)