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

[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>