linux-kernel - Re: [RFC 03/14] x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241106181655.GYZyuyl0zDTTmlMKzz@fat_crate.local>
Date: Wed, 6 Nov 2024 19:16:55 +0100
From: Borislav Petkov <bp@...en8.de>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
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
Subject: Re: [RFC 03/14] x86/apic: Populate .read()/.write() callbacks of
 Secure AVIC driver

On Fri, Sep 13, 2024 at 05:06:54PM +0530, Neeraj Upadhyay wrote:
> @@ -24,6 +25,108 @@ static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
>  }
>  
> +static inline u32 get_reg(char *page, int reg_off)

Just "reg" like the other APICs.

> +{
> +	return READ_ONCE(*((u32 *)(page + reg_off)));
> +}
> +
> +static inline void set_reg(char *page, int reg_off, u32 val)
> +{
> +	WRITE_ONCE(*((u32 *)(page + reg_off)), val);
> +}
> +
> +#define SAVIC_ALLOWED_IRR_OFFSET	0x204
> +
> +static u32 x2apic_savic_read(u32 reg)
> +{
> +	void *backing_page = this_cpu_read(apic_backing_page);
> +
> +	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_ICR:
> +	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:

I'm sure those can be turned into ranges instead of enumerating every single
APIC register...

> +	case APIC_EILVTn(0) ... APIC_EILVTn(3):

Like here.

> +		return get_reg(backing_page, reg);
> +	case APIC_ISR ... APIC_ISR + 0x70:
> +	case APIC_TMR ... APIC_TMR + 0x70:
> +		WARN_ONCE(!IS_ALIGNED(reg, 16), "Reg offset %#x not aligned at 16 bytes", reg);

What's the point of a WARN...

> +		return get_reg(backing_page, reg);

... and then allowing the register access anyway?

> +	/* 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.
> +		 */
> +		WARN_ONCE(!(IS_ALIGNED(reg, 16) || IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)),
> +			  "Misaligned IRR/ALLOWED_IRR reg offset %#x", reg);
> +		return get_reg(backing_page, reg);

Ditto.

And below too.

> +	default:
> +		pr_err("Permission denied: read of Secure AVIC reg offset %#x\n", reg);
> +		return 0;
> +	}
> +}
> +
> +#define SAVIC_NMI_REQ_OFFSET		0x278
> +
> +static void x2apic_savic_write(u32 reg, u32 data)
> +{
> +	void *backing_page = this_cpu_read(apic_backing_page);
> +
> +	switch (reg) {
> +	case APIC_LVTT:
> +	case APIC_LVT0:
> +	case APIC_LVT1:
> +	case APIC_TMICT:
> +	case APIC_TDCR:
> +	case APIC_SELF_IPI:
> +	/* APIC_ID is writable and configured by guest for Secure AVIC */
> +	case APIC_ID:
> +	case APIC_TASKPRI:
> +	case APIC_EOI:
> +	case APIC_SPIV:
> +	case SAVIC_NMI_REQ_OFFSET:
> +	case APIC_ESR:
> +	case APIC_ICR:
> +	case APIC_LVTTHMR:
> +	case APIC_LVTPC:
> +	case APIC_LVTERR:
> +	case APIC_ECTRL:
> +	case APIC_SEOI:
> +	case APIC_IER:
> +	case APIC_EILVTn(0) ... APIC_EILVTn(3):
> +		set_reg(backing_page, reg, data);
> +		break;
> +	/* ALLOWED_IRR offsets are writable */
> +	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
> +		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
> +			set_reg(backing_page, reg, data);
> +			break;
> +		}
> +		fallthrough;
> +	default:
> +		pr_err("Permission denied: write to Secure AVIC reg offset %#x\n", reg);
> +	}
> +}
> +
>  static void x2apic_savic_send_IPI(int cpu, int vector)
>  {
>  	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
> @@ -140,8 +243,8 @@ static struct apic apic_x2apic_savic __ro_after_init = {
>  	.send_IPI_self			= x2apic_send_IPI_self,
>  	.nmi_to_offline_cpu		= true,
>  
> -	.read				= native_apic_msr_read,
> -	.write				= native_apic_msr_write,
> +	.read				= x2apic_savic_read,
> +	.write				= x2apic_savic_write,
>  	.eoi				= native_apic_msr_eoi,
>  	.icr_read			= native_x2apic_icr_read,
>  	.icr_write			= native_x2apic_icr_write,
> -- 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette