[PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[Jim Mattson]

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)
+		return 1;
+
 	if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset)))
 		return 1;
 
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[Krish Sadhukhan]

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>

Re: [PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[Jim Mattson]

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>

Re: [PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[Krish Sadhukhan]

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>

Re: [PATCH] kvm: LAPIC: Restore guard to prevent illegal APIC register access

[Jim Mattson]

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.