Re: [PATCH 1/3] MIPS: Add vulnerabilities infrastructure

[Jiaxun Yang]

在 2020/12/31 23:38, WANG Xuerui 写道:
> Hi Jiaxun,
>
> Overall a nice step towards a more conformant arch/mips! Some nits 
> below though.
>
>
> On 12/30/20 11:23 AM, Jiaxun Yang wrote:
> > Add infrastructure to display CPU vulnerabilities.
> > As most MIPS CPU vendors are dead today and we can't confirm
> > vulnerabilities states with them, we'll display vulnerabilities
> > as "Unknown" by default and override them in cpu-probe.c
> Add trailing period.
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > ---
> >   arch/mips/Kconfig                |  1 +
> >   arch/mips/include/asm/cpu-info.h |  5 ++++
> >   arch/mips/include/asm/cpu.h      |  7 +++++
> >   arch/mips/kernel/Makefile        |  2 +-
> >   arch/mips/kernel/vulnbl.c        | 46 ++++++++++++++++++++++++++++++++
> >   5 files changed, 60 insertions(+), 1 deletion(-)
> >   create mode 100644 arch/mips/kernel/vulnbl.c
> >
> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> > index ef5b2a177b1b..524053b8f769 100644
> > --- a/arch/mips/Kconfig
> > +++ b/arch/mips/Kconfig
> > @@ -24,6 +24,7 @@ config MIPS
> >       select GENERIC_CLOCKEVENTS
> >       select GENERIC_CMOS_UPDATE
> >       select GENERIC_CPU_AUTOPROBE
> > +    select GENERIC_CPU_VULNERABILITIES
> >       select GENERIC_GETTIMEOFDAY
> >       select GENERIC_IOMAP
> >       select GENERIC_IRQ_PROBE
> > diff --git a/arch/mips/include/asm/cpu-info.h 
> > b/arch/mips/include/asm/cpu-info.h
> > index a600670d00e9..1a964dbfc0a8 100644
> > --- a/arch/mips/include/asm/cpu-info.h
> > +++ b/arch/mips/include/asm/cpu-info.h
> > @@ -106,6 +106,11 @@ struct cpuinfo_mips {
> >       unsigned int        guestid_mask;
> >       unsigned int        guestid_cache;
> >   +    /* Vulnerabilities */
> > +    unsigned int        vulnerabilities; /* Vulnerabilities states 
> > that we known */
> > +    unsigned int        vulnerable; /* Vulnerabilities affated */
> > +    unsigned int        mitigations; /* Mitigations */
>
> Could you make the field names a little clearer? Like "known_mask", 
> "affected_mask" and "mitigated_mask"?
>
> Also I wonder if removing the first mask is okay, since if a bit is 
> neither "affected" nor "mitigated" then it must belong to the 
> "unknown" case.

Actually we have no way to mitigate them in kernel for now :-(


> > +
> >   #ifdef CONFIG_CPU_LOONGSON3_CPUCFG_EMULATION
> >       /* CPUCFG data for this CPU, synthesized at probe time.
> >        *
> > diff --git a/arch/mips/include/asm/cpu.h b/arch/mips/include/asm/cpu.h
> > index f5b04e8f6061..3414c9f5464e 100644
> > --- a/arch/mips/include/asm/cpu.h
> > +++ b/arch/mips/include/asm/cpu.h
> > @@ -447,4 +447,11 @@ enum cpu_type_enum {
> >   #define MIPS_ASE_LOONGSON_EXT    0x00002000 /* Loongson EXTensions */
> >   #define MIPS_ASE_LOONGSON_EXT2    0x00004000 /* Loongson EXTensions 
> > R2 */
> >   +/*
> > + * CPU security vulnerabilities
> > + */
> > +#define MIPS_VULNBL_MELTDOWN    BIT(0)
> > +#define MIPS_VULNBL_SPECTRE_V1    BIT(1)
> > +#define MIPS_VULNBL_SPECTRE_V2    BIT(2)
> Looking at the arch/x86 vulnerabilities code, I tend to think that 
> "VULNBL" is not (rather ugly) shorthand for "vulnerability", but 
> "vulnerability blacklist" (!), because they have "VULNWL" for 
> apparently "whitelist". So I suggest writing out "VULNERABILITY" fully 
> for clarity.
> > +
> >   #endif /* _ASM_CPU_H */
> > diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
> > index 13a26d254829..39abc8ead5e0 100644
> > --- a/arch/mips/kernel/Makefile
> > +++ b/arch/mips/kernel/Makefile
> > @@ -8,7 +8,7 @@ extra-y        := head.o vmlinux.lds
> >   obj-y        += cmpxchg.o cpu-probe.o branch.o elf.o entry.o 
> > genex.o idle.o irq.o \
> >              process.o prom.o ptrace.o reset.o setup.o signal.o \
> >              syscall.o time.o topology.o traps.o unaligned.o watch.o \
> > -           vdso.o cacheinfo.o
> > +           vdso.o cacheinfo.o vulnbl.o
> >     ifdef CONFIG_FUNCTION_TRACER
> >   CFLAGS_REMOVE_ftrace.o = -pg
> > diff --git a/arch/mips/kernel/vulnbl.c b/arch/mips/kernel/vulnbl.c
> > new file mode 100644
> > index 000000000000..fc73da6214fe
> > --- /dev/null
> > +++ b/arch/mips/kernel/vulnbl.c
> Same with this filename.
> > @@ -0,0 +1,46 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  Copyright (C) 2020, Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > + *  MIPS CPU vulnerabilities
> > + */
> > +
> > +#include <linux/device.h>
> > +
> > +#include <asm/cpu-info.h>
> > +#include <asm/cpu.h>
> > +
> > +ssize_t cpu_show_meltdown(struct device *dev,
> > +              struct device_attribute *attr, char *buf)
> > +{
> > +    if (!(boot_cpu_data.vulnerabilities & MIPS_VULNBL_MELTDOWN))
> > +        return sprintf(buf, "Unknown\n");
> > +
> > +    if (!(boot_cpu_data.vulnerable & MIPS_VULNBL_MELTDOWN))
> > +        return sprintf(buf, "Not affected\n");
> > +
> > +    return sprintf(buf, "Affected\n");
> Be consistent with other arches and use "Vulnerable"?
> > +}
> > +
> > +ssize_t cpu_show_spectre_v1(struct device *dev,
> > +                struct device_attribute *attr, char *buf)
> > +{
> > +    if (!(boot_cpu_data.vulnerabilities & MIPS_VULNBL_SPECTRE_V1))
> > +        return sprintf(buf, "Unknown\n");
> > +
> > +    if (!(boot_cpu_data.vulnerable & MIPS_VULNBL_SPECTRE_V1))
> > +        return sprintf(buf, "Not affected\n");
> > +
> > +    return sprintf(buf, "Affected\n");
> Same as above.
> > +}
> > +
> > +ssize_t cpu_show_spectre_v2(struct device *dev,
> > +                   struct device_attribute *attr, char *buf)
> > +{
> > +    if (!(boot_cpu_data.vulnerabilities & MIPS_VULNBL_SPECTRE_V2))
> > +        return sprintf(buf, "Unknown\n");
> > +
> > +    if (!(boot_cpu_data.vulnerable & MIPS_VULNBL_SPECTRE_V2))
> > +        return sprintf(buf, "Not affected\n");
> > +
> > +    return sprintf(buf, "Affected\n");
> Same as above.
> > +}