Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Jean Delvare <jdelvare@suse.de>]

Hi Alex,

On Tue, 11 Jan 2022 16:31:51 -0700, Alex Henrie wrote:
> This parameter can only be set before the module is loaded (e.g. by
> passing i2c_i801.block_acpi=1 on the kernel command line).

Before I consider applying this, you'll have to provide a rationale of
why this is needed. Preventing the BIOS from accessing the SMBus is
pretty dangerous, and I can't really think of a situation where you
would want to do that.

Plus, if there's really a reason for doing that, I'd rather have it
implemented as an option in the BIOS itself, than in the kernel driver.

Furthermore, please run ./scripts/checkpatch.pl on your patches and fix
every reported issue before you post them.

> Signed-off-by: Alex Henrie <alexh@vpitech.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 63 +++++++++++++++++++++++++++--------
>  1 file changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 41446f9cc52d..615fd1185b61 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -288,6 +288,11 @@ struct i801_priv {
>  	 * ACPI AML use. Protected by acpi_lock.
>  	 */
>  	bool acpi_reserved;
> +	/*
> +	 * If set to true ACPI AML tried to use SMBus but block_acpi was
> +	 * set. Protected by acpi_lock.
> +	 */
> +	bool acpi_blocked;
>  	struct mutex acpi_lock;
>  };
>  
> @@ -320,6 +325,11 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
>  	"\t\t  0x10  don't use interrupts\n"
>  	"\t\t  0x20  disable SMBus Host Notify ");
>  
> +static bool block_acpi;
> +module_param(block_acpi, bool, S_IRUGO);
> +MODULE_PARM_DESC(block_acpi, "Prevent ACPI AML from accessing SMBus. "
> +	"[0] = allow ACPI access, 1 = deny ACPI access");

I've not seen the square brackets convention for marking the default
value used anywhere else in the kernel. For consistency, please instead
add "(default)" after the default setting.

> +
>  /* Make sure the SMBus host is ready to start transmitting.
>     Return 0 if it is, -EBUSY if it is not. */
>  static int i801_check_pre(struct i801_priv *priv)
> @@ -1616,23 +1626,48 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  	acpi_status status;
>  
>  	/*
> -	 * Once BIOS AML code touches the OpRegion we warn and inhibit any
> -	 * further access from the driver itself. This device is now owned
> -	 * by the system firmware.
> +	 * If BIOS AML code tries to touches the OpRegion we have two options:

Spelling: touches -> touch

> +	 * Warn and inhibit any further access from the driver, or warn and
> +	 * inhibit all access from the BIOS.
>  	 */
>  	mutex_lock(&priv->acpi_lock);
>  
> -	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
> -		priv->acpi_reserved = true;
> -
> -		dev_warn(&pdev->dev, "BIOS is accessing SMBus registers\n");
> -		dev_warn(&pdev->dev, "Driver SMBus register access inhibited\n");
> -
> -		/*
> -		 * BIOS is accessing the host controller so prevent it from
> -		 * suspending automatically from now on.
> -		 */
> -		pm_runtime_get_sync(&pdev->dev);
> +	if (i801_acpi_is_smbus_ioport(priv, address)) {
> +		if (block_acpi) {
> +			/*
> +			 * Refuse to allow the BIOS to use SMBus. SMBus does
> +			 * have a lock bit in the status register that in theory
> +			 * can be used to safely share the SMBus between the
> +			 * BIOS and the kernel, but some badly behaved BIOS
> +			 * implementations don't use it. In that case, the only

It's not really fair to blame the BIOS, considering that the driver
doesn't use it (yet) either. A patch was proposed months ago actually,
reviewing it is still on my to-do list. Could that be an alternative to
your patch?

> +			 * way to ensure continued safe access from the driver
> +			 * is to cripple the BIOS.
> +			 */
> +			if (!priv->acpi_blocked) {
> +				dev_warn(&pdev->dev,
> +					 "BIOS tried to access SMBus registers\n");
> +				dev_warn(&pdev->dev,
> +					 "BIOS SMBus register access inhibited\n");
> +				priv->acpi_blocked = true;
> +			}
> +			mutex_unlock(&priv->acpi_lock);
> +			return -EPERM;
> +		}
> +		if (!priv->acpi_reserved) {
> +			/* This device is now owned by the system firmware. */
> +			priv->acpi_reserved = true;
> +
> +			dev_warn(&pdev->dev,
> +				 "BIOS is accessing SMBus registers\n");
> +			dev_warn(&pdev->dev,
> +				 "Driver SMBus register access inhibited\n");
> +
> +			/*
> +			 * BIOS is accessing the host controller so prevent it
> +			 * from suspending automatically from now on.
> +			 */
> +			pm_runtime_get_sync(&pdev->dev);
> +		}
>  	}
>  
>  	if ((function & ACPI_IO_MASK) == ACPI_READ)


-- 
Jean Delvare
SUSE L3 Support