netdev - Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

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: <063bd012-d377-4d3d-9dcc-57e360d8f462@intel.com>
Date: Thu, 6 Feb 2025 08:46:24 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Choong Yong Liang <yong.liang.choong@...ux.intel.com>,
 Simon Horman <horms@...nel.org>, Jose Abreu <joabreu@...opsys.com>,
 Jose Abreu <Jose.Abreu@...opsys.com>,
 David E Box <david.e.box@...ux.intel.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H . Peter Anvin" <hpa@...or.com>,
 Rajneesh Bhardwaj <irenic.rajneesh@...il.com>,
 David E Box <david.e.box@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin
 <mcoquelin.stm32@...il.com>, Alexandre Torgue
 <alexandre.torgue@...s.st.com>, Jiawen Wu <jiawenwu@...stnetic.com>,
 Mengyuan Lou <mengyuanlou@...-swift.com>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Richard Cochran <richardcochran@...il.com>,
 Serge Semin <fancer.lancer@...il.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 platform-driver-x86@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor
 function and add SoC register access

On 2/6/25 05:18, Choong Yong Liang wrote:
> 
> - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
> - Add support to use IPC command allows host to access SoC registers 
> through PMC firmware that are otherwise inaccessible to the host due
> to security policies.

I'm not quite parsing that second bullet as a complete sentence.

But that sounds scary! Why is the fact that they are "otherwise
inaccessible" relevant here?

...
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 87198d957e2f..631c1f10776c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
>  	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
>  	  implemented under PINCTRL subsystem.
>  
> +config INTEL_PMC_IPC
> +	tristate "Intel Core SoC Power Management Controller IPC mailbox"
> +	depends on ACPI
> +	help
> +	  This option enables sideband register access support for Intel SoC
> +	  power management controller IPC mailbox.
> +
> +	  If you don't require the option or are in doubt, say N.

Could we perhaps beef this up a bit to help users figure out if they
want to turn this on? Really the only word in the entire help text
that's useful is "Intel".

I'm not even sure we *want* to expose this to users. Can we just leave
it as:

	config INTEL_PMC_IPC
		def_tristate n
		depends on ACPI

so that it only gets enabled by the "select" in the other patches?

> + * Authors: Choong Yong Liang <yong.liang.choong@...ux.intel.com>
> + *          David E. Box <david.e.box@...ux.intel.com>

I'd probably just leave the authors bit out. It might have been useful
in the 90's, but that's what git is for today.

> +	obj = buffer.pointer;
> +	/* Check if the number of elements in package is 5 */
> +	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +		const union acpi_object *objs = obj->package.elements;
> +

The comment there is just not super useful. It might be useful to say
*why* the number of elements needs to be 5.

> +EXPORT_SYMBOL(intel_pmc_ipc);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");

Honestly, is this even worth being a module? How much code are we
talking about here?

> diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h b/include/linux/platform_data/x86/intel_pmc_ipc.h
> new file mode 100644
> index 000000000000..d47b89f873fc
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel_pmc_ipc.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Intel Core SoC Power Management Controller Header File
> + *
> + * Copyright (c) 2023, Intel Corporation.
> + * All Rights Reserved.
...

This copyright is a _bit_ funky. It's worth at least saying in the cover
letter that this patch has been sitting untouched for over a year, thus
the old copyright.

Or, if you've done actual work with it, I'd assume the copyright needs
to get updated.

> +struct pmc_ipc_cmd {
> +	u32 cmd;
> +	u32 sub_cmd;
> +	u32 size;
> +	u32 wbuf[4];
> +};
> +
> +/**
> + * intel_pmc_ipc() - PMC IPC Mailbox accessor
> + * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send

You probably don't need to restate the literal type of ipc_cmd.

> + * @rbuf:     Allocated u32[4] array for returned IPC data

The "Allocated" thing here threw me a bit. Does this mean it *must* be
"allocated" as in it comes from kmalloc()? Or can it be on the stack? Or
part of a static variable?

> + * Return: 0 on success. Non-zero on mailbox error
> + */
> +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);

Also, if it can *only* be u32[4], then the best way to declare it is:

struct pmc_ipc_rbuf {
	u32 buf[4];
};

and:

int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd,
		  struct pmc_ipc_rbuf rbuf *rbuf);

Then you don't need a comment saying that it must be a u32[4]. It's
implied in the structure.