| 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: <20190520155658.GA14165@kroah.com>
Date: Mon, 20 May 2019 17:56:58 +0200
From: Greg KH <greg@...ah.com>
To: Liming Sun <lsun@...lanox.com>
Cc: Andy Shevchenko <andy@...radead.org>,
Darren Hart <dvhart@...radead.org>,
Vadim Pasternak <vadimp@...lanox.com>,
David Woods <dwoods@...lanox.com>,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for
Mellanox BlueField Soc
On Fri, May 17, 2019 at 01:49:04PM -0400, Liming Sun wrote:
> This commit adds the bootctl platform driver for Mellanox BlueField
> Soc, which controls the eMMC boot partition swapping and sends SMC
> calls to ATF running at exception level EL3 to program some system
> register. This register is persistent during warm reset and is only
> accessible in secure code which is used to enable the watchdog after
> reboot.
>
> Below are the sequences of typical use case.
>
> 1. User-space tool upgrades one eMMC boot partition and requests
> the boot partition swapping;
>
> 2. The bootctl driver handles this request and sends SMC call
> to ATF. ATF programs register BREADCRUMB0 which has value
> preserved during warm reset. It also programs eMMC to swap
> the boot partition;
>
> 3. After software reset (rebooting), ATF BL1 (BootRom) checks
> register BREADCRUMB0 to enable watchdog if configured;
>
> 4. If booting fails, the watchdog timer will trigger rebooting.
> In such case, ATF Boot ROM will switch the boot partition
> back to the previous one.
>
> Reviewed-by: Vadim Pasternak <vadimp@...lanox.com>
> Signed-off-by: Liming Sun <lsun@...lanox.com>
> ---
> v4->v5:
> Fixes for comments from Andy:
> - Added ABI documentation;
> - Remove the extra 'const' in mlxbf_bootctl_svc_uuid_str definition;
> - Remove the mlxbf_bootctl_smc_call0() MACRO to use function
> call directly;
> - Return more descriptive string ('invalid action') in
> mlxbf_bootctl_reset_action_to_string();
> - Check return value of the mlxbf_bootctl_smc() in function
> post_reset_wdog_show() and reset_action_show();
> - Revise the sprintf() line in reset_action_show() into one line;
> - Move the 'action = ' assignment out of the declarations
> in reset_action_store() and check return value of
> mlxbf_bootctl_reset_action_to_val() in this function;
> - Removed Redundant parens in reset_action_store();
> - Check return code of the smc_call in second_reset_action_show(),
> merge the 'name = ' assignment into the sprintf() directly;
> - Move the 'action = ' assignment out of the declaration block
> in second_reset_action_store();
> - Remove redundant blank line and parents in lifecycle_state_show();
> - Split declaration and assignment in secure_boot_fuse_state_show();
> - use BIT() in secure_boot_fuse_state_show() and simplify code in
> this function to split the logic of message choice and flag flip;
> Also removed the 'key !=0 ' check since it's not needed;
> - Added comma in mlxbf_bootctl_attr_group definition.
> - Removed un-needed '& 0xFF' logic in mlxbf_bootctl_guid_match();
> - Use temp variable for the result of the smc call in
> mlxbf_bootctl_probe();
> - Use dev_warn() instead of dev_err() in mlxbf_bootctl_probe();
> - Removed the ACPI_PTR usage in the mlxbf_bootctl_driver definition;
> Fixes for comments from Vadim:
> - Move mlxbf-bootctl.o to the top of the Makefile;
> - Fix to use the 'ret' value instead of another mlxbf_bootctl_smc() call;
> - Fix the 'declaration inside the block' in secure_boot_fuse_state_show();
> - Use ATTRIBUTE_GROUPS() for the attribute definition;
> - Use single line for a comment in mlxbf-bootctl.h
> - Use KernelVersion 5.3 instead of 5.2.2 in the ABI doc;
> - Use common utility function for the show and store of reset_action
> and second_reset_action.
> New changes:
> - Rename mlxbf_bootctl_smc_call1() to mlxbf_bootctl_smc() to
> shorten the name so some statement could be make into one line;
> - Fixed the MODULE_LICENSE();
> - Added more comments in secure_boot_fuse_state_show();
> v4: Update Kconfig for the dependency on ACPI.
> Fixed a typo which caused build error for kernel module.
> v2->v3:
> Fixes for comments from Andy:
> - More coding style fixes;
> - Revised the uuid matching code.
> v2: Fix the Kconfig and coding styles, propagate errors to caller.
> v1: Initial version.
> ---
> drivers/platform/mellanox/Kconfig | 12 ++
> drivers/platform/mellanox/Makefile | 1 +
> drivers/platform/mellanox/mlxbf-bootctl.c | 311 ++++++++++++++++++++++++++++++
> drivers/platform/mellanox/mlxbf-bootctl.h | 103 ++++++++++
> 4 files changed, 427 insertions(+)
> create mode 100644 drivers/platform/mellanox/mlxbf-bootctl.c
> create mode 100644 drivers/platform/mellanox/mlxbf-bootctl.h
>
> diff --git a/drivers/platform/mellanox/Kconfig b/drivers/platform/mellanox/Kconfig
> index 530fe7e..386336d 100644
> --- a/drivers/platform/mellanox/Kconfig
> +++ b/drivers/platform/mellanox/Kconfig
> @@ -44,4 +44,16 @@ config MLXBF_TMFIFO
> platform driver support for the TmFifo which supports console
> and networking based on the virtio framework.
>
> +config MLXBF_BOOTCTL
> + tristate "Mellanox BlueField Firmware Boot Control driver"
> + depends on ARM64
CONFIG_TEST so that we can all build this to ensure it does nto break?
> + depends on ACPI
> + help
> + The Mellanox BlueField firmware implements functionality to
> + request swapping the primary and alternate eMMC boot partition,
> + and to set up a watchdog that can undo that swap if the system
> + does not boot up correctly. This driver provides sysfs access
> + to the userspace tools, to be used in conjunction with the eMMC
> + device driver to do necessary initial swap of the boot partition.
> +
> endif # MELLANOX_PLATFORM
> diff --git a/drivers/platform/mellanox/Makefile b/drivers/platform/mellanox/Makefile
> index a229bda1..499623c 100644
> --- a/drivers/platform/mellanox/Makefile
> +++ b/drivers/platform/mellanox/Makefile
> @@ -3,6 +3,7 @@
> # Makefile for linux/drivers/platform/mellanox
> # Mellanox Platform-Specific Drivers
> #
> +obj-$(CONFIG_MLXBF_BOOTCTL) += mlxbf-bootctl.o
> obj-$(CONFIG_MLXBF_TMFIFO) += mlxbf-tmfifo.o
> obj-$(CONFIG_MLXREG_HOTPLUG) += mlxreg-hotplug.o
> obj-$(CONFIG_MLXREG_IO) += mlxreg-io.o
> diff --git a/drivers/platform/mellanox/mlxbf-bootctl.c b/drivers/platform/mellanox/mlxbf-bootctl.c
> new file mode 100644
> index 0000000..842b991
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf-bootctl.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Mellanox boot control driver
> + *
> + * This driver provides a sysfs interface for systems management
> + * software to manage reset-time actions.
> + *
> + * Copyright (C) 2019 Mellanox Technologies
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "mlxbf-bootctl.h"
> +
> +#define MLXBF_BOOTCTL_SB_SECURE_MASK 0x03
> +#define MLXBF_BOOTCTL_SB_TEST_MASK 0x0c
> +
> +#define MLXBF_SB_KEY_NUM 4
> +
> +/* UUID used to probe ATF service. */
> +static const char *mlxbf_bootctl_svc_uuid_str =
> + "89c036b4-e7d7-11e6-8797-001aca00bfc4";
> +
> +struct mlxbf_bootctl_name {
> + u32 value;
> + const char *name;
> +};
> +
> +static struct mlxbf_bootctl_name boot_names[] = {
> + { MLXBF_BOOTCTL_EXTERNAL, "external" },
> + { MLXBF_BOOTCTL_EMMC, "emmc" },
> + { MLNX_BOOTCTL_SWAP_EMMC, "swap_emmc" },
> + { MLXBF_BOOTCTL_EMMC_LEGACY, "emmc_legacy" },
> + { MLXBF_BOOTCTL_NONE, "none" },
> +};
> +
> +static const char * const mlxbf_bootctl_lifecycle_states[] = {
> + [0] = "Production",
> + [1] = "GA Secured",
> + [2] = "GA Non-Secured",
> + [3] = "RMA",
> +};
> +
> +/* ARM SMC call which is atomic and no need for lock. */
> +static int mlxbf_bootctl_smc(unsigned int smc_op, int smc_arg)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(smc_op, smc_arg, 0, 0, 0, 0, 0, 0, &res);
> +
> + return res.a0;
> +}
> +
> +/* Return the action in integer or an error code. */
> +static int mlxbf_bootctl_reset_action_to_val(const char *action)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(boot_names); i++)
> + if (sysfs_streq(boot_names[i].name, action))
> + return boot_names[i].value;
> +
> + return -EINVAL;
> +}
> +
> +/* Return the action in string. */
> +static const char *mlxbf_bootctl_action_to_string(int action)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(boot_names); i++)
> + if (boot_names[i].value == action)
> + return boot_names[i].name;
> +
> + return "invalid action";
> +}
> +
> +static ssize_t post_reset_wdog_show(struct device_driver *drv, char *buf)
> +{
> + int ret;
> +
> + ret = mlxbf_bootctl_smc(MLXBF_BOOTCTL_GET_POST_RESET_WDOG, 0);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t post_reset_wdog_store(struct device_driver *drv,
> + const char *buf, size_t count)
> +{
> + unsigned long value;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &value);
> + if (ret)
> + return ret;
> +
> + ret = mlxbf_bootctl_smc(MLXBF_BOOTCTL_SET_POST_RESET_WDOG, value);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +
> +static ssize_t mlxbf_bootctl_show(int smc_op, char *buf)
> +{
> + int action;
> +
> + action = mlxbf_bootctl_smc(smc_op, 0);
> + if (action < 0)
> + return action;
> +
> + return sprintf(buf, "%s\n", mlxbf_bootctl_action_to_string(action));
> +}
> +
> +static int mlxbf_bootctl_store(int smc_op, const char *buf, size_t count)
> +{
> + int ret, action;
> +
> + action = mlxbf_bootctl_reset_action_to_val(buf);
> + if (action < 0)
> + return action;
> +
> + ret = mlxbf_bootctl_smc(smc_op, action);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +
> +static ssize_t reset_action_show(struct device_driver *drv, char *buf)
> +{
> + return mlxbf_bootctl_show(MLXBF_BOOTCTL_GET_RESET_ACTION, buf);
> +}
> +
> +static ssize_t reset_action_store(struct device_driver *drv,
> + const char *buf, size_t count)
> +{
> + return mlxbf_bootctl_store(MLXBF_BOOTCTL_SET_RESET_ACTION, buf, count);
> +}
> +
> +static ssize_t second_reset_action_show(struct device_driver *drv, char *buf)
> +{
> + return mlxbf_bootctl_show(MLXBF_BOOTCTL_GET_SECOND_RESET_ACTION, buf);
> +}
> +
> +static ssize_t second_reset_action_store(struct device_driver *drv,
> + const char *buf, size_t count)
> +{
> + return mlxbf_bootctl_store(MLXBF_BOOTCTL_SET_SECOND_RESET_ACTION, buf,
> + count);
> +}
> +
> +static ssize_t lifecycle_state_show(struct device_driver *drv, char *buf)
> +{
> + int lc_state;
> +
> + lc_state = mlxbf_bootctl_smc(MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> + MLXBF_BOOTCTL_FUSE_STATUS_LIFECYCLE);
> + if (lc_state < 0)
> + return lc_state;
> +
> + lc_state &=
> + MLXBF_BOOTCTL_SB_TEST_MASK | MLXBF_BOOTCTL_SB_SECURE_MASK;
> +
> + /*
> + * If the test bits are set, we specify that the current state may be
> + * due to using the test bits.
> + */
> + if (lc_state & MLXBF_BOOTCTL_SB_TEST_MASK) {
> + lc_state &= MLXBF_BOOTCTL_SB_SECURE_MASK;
> +
> + return sprintf(buf, "%s(test)\n",
> + mlxbf_bootctl_lifecycle_states[lc_state]);
Ick, why is (test) used here? Who is going to parse this? Shouldn't
"test" be a separate sysfs file?
> + }
> +
> + return sprintf(buf, "%s\n", mlxbf_bootctl_lifecycle_states[lc_state]);
> +}
> +
> +static ssize_t secure_boot_fuse_state_show(struct device_driver *drv, char *buf)
> +{
> + int burnt, valid, key, key_state, buf_len = 0, upper_key_used = 0;
> + const char *status;
> +
> + key_state = mlxbf_bootctl_smc(MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> + MLXBF_BOOTCTL_FUSE_STATUS_KEYS);
> + if (key_state < 0)
> + return key_state;
> +
> + /*
> + * key_state contains the bits for 4 Key versions, loaded from eFuses
> + * after a hard reset. Lower 4 bits are a thermometer code indicating
> + * key programming has started for key n (0000 = none, 0001 = version 0,
> + * 0011 = version 1, 0111 = version 2, 1111 = version 3). Upper 4 bits
> + * are a thermometer code indicating key programming has completed for
> + * key n (same encodings as the start bits). This allows for detection
> + * of an interruption in the progamming process which has left the key
> + * partially programmed (and thus invalid). The process is to burn the
> + * eFuse for the new key start bit, burn the key eFuses, then burn the
> + * eFuse for the new key complete bit.
> + *
> + * For example 0000_0000: no key valid, 0001_0001: key version 0 valid,
> + * 0011_0011: key 1 version valid, 0011_0111: key version 2 started
> + * programming but did not complete, etc. The most recent key for which
> + * both start and complete bit is set is loaded. On soft reset, this
> + * register is not modified.
> + */
> + for (key = MLXBF_SB_KEY_NUM - 1; key >= 0; key--) {
> + burnt = key_state & BIT(key);
> + valid = key_state & BIT(key + MLXBF_SB_KEY_NUM);
> +
> + if (burnt && valid)
> + upper_key_used = 1;
> +
> + if (upper_key_used) {
> + if (burnt)
> + status = valid ? "Used" : "Wasted";
> + else
> + status = valid ? "Invalid" : "Skipped";
> + } else {
> + if (burnt)
> + status = valid ? "InUse" : "Incomplete";
> + else
> + status = valid ? "Invalid" : "Free";
> + }
> + buf_len += sprintf(buf + buf_len, "%d:%s ", key, status);
> + }
> + buf_len += sprintf(buf + buf_len, "\n");
sysfs files are one-value-per-file, not a whole bunch of lists of
different values. Please fix this up.
> +
> + return buf_len;
> +}
> +
> +static DRIVER_ATTR_RW(post_reset_wdog);
> +static DRIVER_ATTR_RW(reset_action);
> +static DRIVER_ATTR_RW(second_reset_action);
> +static DRIVER_ATTR_RO(lifecycle_state);
> +static DRIVER_ATTR_RO(secure_boot_fuse_state);
> +
> +static struct attribute *mlxbf_bootctl_attrs[] = {
> + &driver_attr_post_reset_wdog.attr,
> + &driver_attr_reset_action.attr,
> + &driver_attr_second_reset_action.attr,
> + &driver_attr_lifecycle_state.attr,
> + &driver_attr_secure_boot_fuse_state.attr,
> + NULL
> +};
> +
> +ATTRIBUTE_GROUPS(mlxbf_bootctl);
> +
> +static const struct acpi_device_id mlxbf_bootctl_acpi_ids[] = {
> + {"MLNXBF04", 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, mlxbf_bootctl_acpi_ids);
> +
> +static bool mlxbf_bootctl_guid_match(const guid_t *guid,
> + const struct arm_smccc_res *res)
> +{
> + guid_t id = GUID_INIT(res->a0, res->a1, res->a1 >> 16,
> + res->a2, res->a2 >> 8, res->a2 >> 16,
> + res->a2 >> 24, res->a3, res->a3 >> 8,
> + res->a3 >> 16, res->a3 >> 24);
> +
> + return guid_equal(guid, &id);
> +}
> +
> +static int mlxbf_bootctl_probe(struct platform_device *pdev)
> +{
> + struct arm_smccc_res res = { 0 };
> + guid_t guid;
> + int ret;
> +
> + /* Ensure we have the UUID we expect for this service. */
> + arm_smccc_smc(MLXBF_BOOTCTL_SIP_SVC_UID, 0, 0, 0, 0, 0, 0, 0, &res);
> + guid_parse(mlxbf_bootctl_svc_uuid_str, &guid);
> + if (!mlxbf_bootctl_guid_match(&guid, &res))
> + return -ENODEV;
> +
> + /*
> + * When watchdog is used, it sets boot mode to MLXBF_BOOTCTL_SWAP_EMMC
> + * in case of boot failures. However it doesn't clear the state if there
> + * is no failure. Restore the default boot mode here to avoid any
> + * unnecessary boot partition swapping.
> + */
> + ret = mlxbf_bootctl_smc(MLXBF_BOOTCTL_SET_RESET_ACTION,
> + MLXBF_BOOTCTL_EMMC);
> + if (ret < 0)
> + dev_warn(&pdev->dev, "Unable to reset the EMMC boot mode\n");
> +
> + return 0;
> +}
> +
> +static struct platform_driver mlxbf_bootctl_driver = {
> + .probe = mlxbf_bootctl_probe,
> + .driver = {
> + .name = "mlxbf-bootctl",
> + .groups = mlxbf_bootctl_groups,
> + .acpi_match_table = mlxbf_bootctl_acpi_ids,
Why is an acpi driver a platform driver? Isn't there a "real" acpi
driver interface you should be tieing into instead?
Only use a platform driver as an absolute last resort. I don't think
that is the case here.
thanks,
greg k-h
Powered by blists - more mailing lists