| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <964f3885-059e-4ab0-b8fc-1b949f0b853b@amd.com>
Date: Tue, 19 Aug 2025 09:45:02 +0530
From: "Upadhyay, Neeraj" <neeraj.upadhyay@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
dave.hansen@...ux.intel.com, Thomas.Lendacky@....com, nikunj@....com,
Santosh.Shukla@....com, Vasant.Hegde@....com, Suravee.Suthikulpanit@....com,
David.Kaplan@....com, x86@...nel.org, hpa@...or.com, peterz@...radead.org,
seanjc@...gle.com, pbonzini@...hat.com, kvm@...r.kernel.org,
kirill.shutemov@...ux.intel.com, huibo.wang@....com, naveen.rao@....com,
francescolavra.fl@...il.com, tiala@...rosoft.com
Subject: Re: [PATCH v9 03/18] x86/apic: Populate .read()/.write() callbacks of
Secure AVIC driver
On 8/18/2025 4:56 PM, Borislav Petkov wrote:
> On Mon, Aug 11, 2025 at 03:14:29PM +0530, Neeraj Upadhyay wrote:
>> Add read() and write() APIC callback functions to read and write x2APIC
>> registers directly from the guest APIC backing page of a vCPU.
>>
>> The x2APIC registers are mapped at an offset within the guest APIC
>> backing page which is same as their x2APIC MMIO offset. Secure AVIC
>> adds new registers such as ALLOWED_IRRs (which are at 4-byte offset
>> within the IRR register offset range) and NMI_REQ to the APIC register
>> space.
>>
>> When Secure AVIC is enabled, guest's rdmsr/wrmsr of APIC registers
>> result in VC exception (for non-accelerated register accesses) with
>
> s/VC/#VC/g since you're talking about an exception vector.
>
Ok
>> error code VMEXIT_AVIC_NOACCEL. The VC exception handler can read/write
>> the x2APIC register in the guest APIC backing page to complete the
>> rdmsr/wrmsr.
>
> All x86 insns in caps pls: RDMSR/WRMSR.
>
Ok
>> +static u32 savic_read(u32 reg)
>> +{
>> + void *ap = this_cpu_ptr(secure_avic_page);
>> +
>> + /*
>> + * When Secure AVIC is enabled, rdmsr/wrmsr of APIC registers
>> + * result in VC exception (for non-accelerated register accesses)
>> + * with VMEXIT_AVIC_NOACCEL error code. The VC exception handler
>> + * can read/write the x2APIC register in the guest APIC backing page.
>> + * Since doing this would increase the latency of accessing x2APIC
>> + * registers, instead of doing rdmsr/wrmsr based accesses and
>> + * handling apic register reads/writes in VC exception, the read()
>
> s/apic/APIC/g
>
> Please be consistent across the whole set. Acronyms are in all caps. Insn
> names too.
>
Ok
>> + * and write() callbacks directly read/write APIC register from/to
>> + * the vCPU APIC backing page.
>> + */
>
> Move that comment above the function. And also split it in paragraphs: when it
> becomes more than 4-5 lines, split the next one with a new line.
>
Ok
>> + switch (reg) {
>> + case APIC_LVTT:
>> + case APIC_TMICT:
>> + case APIC_TMCCT:
>> + case APIC_TDCR:
>> + case APIC_ID:
>> + case APIC_LVR:
>> + case APIC_TASKPRI:
>> + case APIC_ARBPRI:
>> + case APIC_PROCPRI:
>> + case APIC_LDR:
>> + case APIC_SPIV:
>> + case APIC_ESR:
>> + case APIC_LVTTHMR:
>> + case APIC_LVTPC:
>> + case APIC_LVT0:
>> + case APIC_LVT1:
>> + case APIC_LVTERR:
>> + case APIC_EFEAT:
>> + case APIC_ECTRL:
>> + case APIC_SEOI:
>> + case APIC_IER:
>> + case APIC_EILVTn(0) ... APIC_EILVTn(3):
>> + return apic_get_reg(ap, reg);
>> + case APIC_ICR:
>> + return (u32) apic_get_reg64(ap, reg);
> ^
>
> no need for that space.
>
Ok, will remove.
>> + case APIC_ISR ... APIC_ISR + 0x70:
>> + case APIC_TMR ... APIC_TMR + 0x70:
>> + if (WARN_ONCE(!IS_ALIGNED(reg, 16),
>> + "APIC reg read offset 0x%x not aligned at 16 bytes", reg))
>> + return 0;
>> + return apic_get_reg(ap, reg);
>> + /* IRR and ALLOWED_IRR offset range */
>> + case APIC_IRR ... APIC_IRR + 0x74:
>> + /*
>> + * Either aligned at 16 bytes for valid IRR reg offset or a
>> + * valid Secure AVIC ALLOWED_IRR offset.
>> + */
>> + if (WARN_ONCE(!(IS_ALIGNED(reg, 16) ||
>> + IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16)),
>> + "Misaligned IRR/ALLOWED_IRR APIC reg read offset 0x%x", reg))
>
> What is that second thing supposed to catch?
>
> reg can be aligned to 16 but reg - SAVIC_ALLOWED_IRR cannot be?
>
APIC_IRR register offsets are:
#Offset #bits Description
0x200 31:0 vectors 0-31
0x210 31:0 vectors 32-63
...
0x270 31:0 vectors 224-255
ALLOWED_IRR register offsets are:
#Offset #bits Description
0x204 31:0 vectors 0-31
0x214 31:0 vectors 32-63
...
0x274 31:0 vectors 224-255
IS_ALIGNED(reg, 16) is when 'reg' is an APIC_IRR register, which are 16
byte aligned.
IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16) is for the case when 'reg' is
a SAVIC_ALLOWED_IRR register. which are at 16 byte strides from the
SAVIC_ALLOWED_IRR base offset. Expected values of (reg -
SAVIC_ALLOWED_IRR) are 0, 0x10, 0x20, ..., 0x70.
If both checks fail, that is a invalid offset ('!' is on the final ORed
value).
!(IS_ALIGNED(reg, 16) || IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16))
> I can't follow the comment... perhaps write it out and not try to be clever.
>
Maybe change it to below?
/*
* Valid APIC_IRR/SAVIC_ALLOWED_IRR registers are at 16 bytes strides
* from their respective base offset.
*/
if (WARN_ONCE(!(IS_ALIGNED(reg - APIC_IRR, 16) ||
IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16)),
"Misaligned APIC_IRR/ALLOWED_IRR APIC register read
offset 0x%x",
reg))
>> + return 0;
>> + return apic_get_reg(ap, reg);
>> + default:
>> + pr_err("Permission denied: read of Secure AVIC reg offset 0x%x\n", reg);
>
> Permission denied?
>
> "Error reading unknown Secure AVIC reg offset..."
>
> I'd say.
>
Ok, yes it is clearer.
- Neeraj
Powered by blists - more mailing lists