| Message ID | 20210609215111.4142972-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: LAPIC: Restore guard to prevent illegal APIC register access | expand |
On 6/9/21 2:51 PM, Jim Mattson wrote: > Per the SDM, "any access that touches bytes 4 through 15 of an APIC > register may cause undefined behavior and must not be executed." > Worse, such an access in kvm_lapic_reg_read can result in a leak of > kernel stack contents. Prior to commit 01402cf81051 ("kvm: LAPIC: > write down valid APIC registers"), such an access was explicitly > disallowed. Restore the guard that was removed in that commit. > > Fixes: 01402cf81051 ("kvm: LAPIC: write down valid APIC registers") > Signed-off-by: Jim Mattson <jmattson@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > arch/x86/kvm/lapic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index c0ebef560bd1..32fb82bbd63f 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1410,6 +1410,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, > if (!apic_x2apic_mode(apic)) > valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI); > > + if (alignment + len > 4) It will be useful for debugging if the apic_debug() call is added back in. > + return 1; > + > if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset))) > return 1; > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Wed, Jun 9, 2021 at 5:45 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > On 6/9/21 2:51 PM, Jim Mattson wrote: > > Per the SDM, "any access that touches bytes 4 through 15 of an APIC > > register may cause undefined behavior and must not be executed." > > Worse, such an access in kvm_lapic_reg_read can result in a leak of > > kernel stack contents. Prior to commit 01402cf81051 ("kvm: LAPIC: > > write down valid APIC registers"), such an access was explicitly > > disallowed. Restore the guard that was removed in that commit. > > > > Fixes: 01402cf81051 ("kvm: LAPIC: write down valid APIC registers") > > Signed-off-by: Jim Mattson <jmattson@google.com> > > Reported-by: syzbot <syzkaller@googlegroups.com> > > --- > > arch/x86/kvm/lapic.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index c0ebef560bd1..32fb82bbd63f 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1410,6 +1410,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, > > if (!apic_x2apic_mode(apic)) > > valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI); > > > > + if (alignment + len > 4) > > It will be useful for debugging if the apic_debug() call is added back in. Are you suggesting that I should revert commit 0d88800d5472 ("kvm: x86: ioapic and apic debug macros cleanup")? > > + return 1; > > + > > if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset))) > > return 1; > > > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On 6/9/21 6:48 PM, Jim Mattson wrote: > On Wed, Jun 9, 2021 at 5:45 PM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> >> On 6/9/21 2:51 PM, Jim Mattson wrote: >>> Per the SDM, "any access that touches bytes 4 through 15 of an APIC >>> register may cause undefined behavior and must not be executed." >>> Worse, such an access in kvm_lapic_reg_read can result in a leak of >>> kernel stack contents. Prior to commit 01402cf81051 ("kvm: LAPIC: >>> write down valid APIC registers"), such an access was explicitly >>> disallowed. Restore the guard that was removed in that commit. >>> >>> Fixes: 01402cf81051 ("kvm: LAPIC: write down valid APIC registers") >>> Signed-off-by: Jim Mattson <jmattson@google.com> >>> Reported-by: syzbot <syzkaller@googlegroups.com> >>> --- >>> arch/x86/kvm/lapic.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>> index c0ebef560bd1..32fb82bbd63f 100644 >>> --- a/arch/x86/kvm/lapic.c >>> +++ b/arch/x86/kvm/lapic.c >>> @@ -1410,6 +1410,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, >>> if (!apic_x2apic_mode(apic)) >>> valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI); >>> >>> + if (alignment + len > 4) >> It will be useful for debugging if the apic_debug() call is added back in. > Are you suggesting that I should revert commit 0d88800d5472 ("kvm: > x86: ioapic and apic debug macros cleanup")? Oh, I wasn't aware that commit 0d88800d5472 had removed the debug macros. The tracepoint in kvm_lapic_reg_read() fires after these error checks pass. A printk may be useful. Or perhaps move the tracepoint up ? > >>> + return 1; >>> + >>> if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset))) >>> return 1; >>> >> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Wed, Jun 9, 2021 at 7:45 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > On 6/9/21 6:48 PM, Jim Mattson wrote: > > On Wed, Jun 9, 2021 at 5:45 PM Krish Sadhukhan > > <krish.sadhukhan@oracle.com> wrote: > >> > >> On 6/9/21 2:51 PM, Jim Mattson wrote: > >>> Per the SDM, "any access that touches bytes 4 through 15 of an APIC > >>> register may cause undefined behavior and must not be executed." > >>> Worse, such an access in kvm_lapic_reg_read can result in a leak of > >>> kernel stack contents. Prior to commit 01402cf81051 ("kvm: LAPIC: > >>> write down valid APIC registers"), such an access was explicitly > >>> disallowed. Restore the guard that was removed in that commit. > >>> > >>> Fixes: 01402cf81051 ("kvm: LAPIC: write down valid APIC registers") > >>> Signed-off-by: Jim Mattson <jmattson@google.com> > >>> Reported-by: syzbot <syzkaller@googlegroups.com> > >>> --- > >>> arch/x86/kvm/lapic.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > >>> index c0ebef560bd1..32fb82bbd63f 100644 > >>> --- a/arch/x86/kvm/lapic.c > >>> +++ b/arch/x86/kvm/lapic.c > >>> @@ -1410,6 +1410,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, > >>> if (!apic_x2apic_mode(apic)) > >>> valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI); > >>> > >>> + if (alignment + len > 4) > >> It will be useful for debugging if the apic_debug() call is added back in. > > Are you suggesting that I should revert commit 0d88800d5472 ("kvm: > > x86: ioapic and apic debug macros cleanup")? > > > Oh, I wasn't aware that commit 0d88800d5472 had removed the debug > macros. The tracepoint in kvm_lapic_reg_read() fires after these error > checks pass. A printk may be useful. Or perhaps move the tracepoint up ? It sounds like you disagree with the claim in commit 0d88800d5472 ("kvm: x86: ioapic and apic debug macros cleanup") that "kvm tracepoints are enough for debugging." I'll let you argue that point with Yi and Paolo separately, as it seems orthogonal to this change. My personal opinion is that messages that get written to syslog are next to useless. Unless the message goes out to userspace, I have no way of getting the message to my end customers. Similarly, while tracepoints may be useful for developers, they are useless in production, and since I can't usually get my hands on a customer workload to run in a development environment, tracepoints also have quite limited usefulness.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index c0ebef560bd1..32fb82bbd63f 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1410,6 +1410,9 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, if (!apic_x2apic_mode(apic)) valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI); + if (alignment + len > 4) + return 1; + if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset))) return 1;
Per the SDM, "any access that touches bytes 4 through 15 of an APIC register may cause undefined behavior and must not be executed." Worse, such an access in kvm_lapic_reg_read can result in a leak of kernel stack contents. Prior to commit 01402cf81051 ("kvm: LAPIC: write down valid APIC registers"), such an access was explicitly disallowed. Restore the guard that was removed in that commit. Fixes: 01402cf81051 ("kvm: LAPIC: write down valid APIC registers") Signed-off-by: Jim Mattson <jmattson@google.com> Reported-by: syzbot <syzkaller@googlegroups.com> --- arch/x86/kvm/lapic.c | 3 +++ 1 file changed, 3 insertions(+)