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: <20200910201040.GU2997@nanopsycho.orion>
Date:   Thu, 10 Sep 2020 22:10:40 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [net-next v4 3/5] devlink: introduce flash update overwrite mask

Thu, Sep 10, 2020 at 12:26:51AM CEST, jacob.e.keller@...el.com wrote:
>Sections of device flash may contain settings or device identifying
>information. When performing a flash update, it is generally expected
>that these settings and identifiers are not overwritten.
>
>However, it may sometimes be useful to allow overwriting these fields
>when performing a flash update. Some examples include, 1) customizing
>the initial device config on first programming, such as overwriting
>default device identifying information, or 2) reverting a device
>configuration to known good state provided in the new firmware image, or
>3) in case it is suspected that current firmware logic for managing the
>preservation of fields during an update is broken.
>
>Although some devices are able to completely separate these types of
>settings and fields into separate components, this is not true for all
>hardware.
>
>To support controlling this behavior, a new
>DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK is defined. This is an
>nla_bitfield32 which will define what subset of fields in a component
>should be overwritten during an update.
>
>If no bits are specified, or of the overwrite mask is not provided, then
>an update should not overwrite anything, and should maintain the
>settings and identifiers as they are in the previous image.
>
>If the overwrite mask has the DEVLINK_FLASH_OVERWRITE_SETTINGS bit set,
>then the device should be configured to overwrite any of the settings in
>the requested component with settings found in the provided image.
>
>Similarly, if the DEVLINK_FLASH_OVERWRITE_IDENTIFIERS bit is set, the
>device should be configured to overwrite any device identifiers in the
>requested component with the identifiers from the image.
>
>Multiple overwrite modes may be combined to indicate that a combination
>of the set of fields that should be overwritten.
>
>Drivers which support the new overwrite mask must set the
>DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK in the
>supported_flash_update_params field of their devlink_ops.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>---
>Changes since v3
>* split netdevsim driver changes to a new patch
>* fixed a double-the typo in the documentation
>
> .../networking/devlink/devlink-flash.rst      | 28 +++++++++++++++++++
> include/net/devlink.h                         |  4 ++-
> include/uapi/linux/devlink.h                  | 25 +++++++++++++++++
> net/core/devlink.c                            | 17 ++++++++++-
> 4 files changed, 72 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst
>index 40a87c0222cb..603e732f00cc 100644
>--- a/Documentation/networking/devlink/devlink-flash.rst
>+++ b/Documentation/networking/devlink/devlink-flash.rst
>@@ -16,6 +16,34 @@ Note that the file name is a path relative to the firmware loading path
> (usually ``/lib/firmware/``). Drivers may send status updates to inform
> user space about the progress of the update operation.
> 
>+Overwrite Mask
>+==============
>+
>+The ``devlink-flash`` command allows optionally specifying a mask indicating
>+how the device should handle subsections of flash components when updating.
>+This mask indicates the set of sections which are allowed to be overwritten.
>+
>+.. list-table:: List of overwrite mask bits
>+   :widths: 5 95
>+
>+   * - Name
>+     - Description
>+   * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS``
>+     - Indicates that the device should overwrite settings in the components
>+       being updated with the settings found in the provided image.
>+   * - ``DEVLINK_FLASH_OVERWRITE_IDENTIFIERS``
>+     - Indicates that the device should overwrite identifiers in the
>+       components being updated with the identifiers found in the provided
>+       image. This includes MAC addresses, serial IDs, and similar device
>+       identifiers.
>+
>+Multiple overwrite bits may be combined and requested together. If no bits
>+are provided, it is expected that the device only update firmware binaries
>+in the components being updated. Settings and identifiers are expected to be
>+preserved across the update. A device may not support every combination and
>+the driver for such a device must reject any combination which cannot be
>+faithfully implemented.
>+
> Firmware Loading
> ================
> 
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 3384e901bbf0..ff4638f7e547 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -543,9 +543,11 @@ enum devlink_param_generic_id {
> struct devlink_flash_update_params {
> 	const char *file_name;
> 	const char *component;
>+	u32 overwrite_mask;
> };
> 
>-#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT	BIT(0)
>+#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT		BIT(0)
>+#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
> 
> struct devlink_region;
> struct devlink_info_req;
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 40d35145c879..19a573566359 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -228,6 +228,28 @@ enum {
> 	DEVLINK_ATTR_STATS_MAX = __DEVLINK_ATTR_STATS_MAX - 1
> };
> 
>+/* Specify what sections of a flash component can be overwritten when
>+ * performing an update. Overwriting of firmware binary sections is always
>+ * implicitly assumed to be allowed.
>+ *
>+ * Each section must be documented in
>+ * Documentation/networking/devlink/devlink-flash.rst
>+ *
>+ */
>+enum {
>+	DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT,
>+	DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT,
>+
>+	__DEVLINK_FLASH_OVERWRITE_MAX_BIT,
>+	DEVLINK_FLASH_OVERWRITE_MAX_BIT = __DEVLINK_FLASH_OVERWRITE_MAX_BIT - 1
>+};
>+
>+#define DEVLINK_FLASH_OVERWRITE_SETTINGS BIT(DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT)
>+#define DEVLINK_FLASH_OVERWRITE_IDENTIFIERS BIT(DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT)
>+
>+#define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
>+	(BIT(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
>+
> /**
>  * enum devlink_trap_action - Packet trap action.
>  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
>@@ -460,6 +482,9 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_PORT_EXTERNAL,		/* u8 */
> 	DEVLINK_ATTR_PORT_CONTROLLER_NUMBER,	/* u32 */
>+
>+	DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK,	/* bitfield32 */
>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index c61f9c8205f6..d0d38ca17ea8 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3125,8 +3125,8 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 				       struct genl_info *info)
> {
> 	struct devlink_flash_update_params params = {};
>+	struct nlattr *nla_component, *nla_overwrite;
> 	struct devlink *devlink = info->user_ptr[0];
>-	struct nlattr *nla_component;
> 	u32 supported_params;
> 
> 	if (!devlink->ops->flash_update)
>@@ -3149,6 +3149,19 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 		params.component = nla_data(nla_component);
> 	}
> 
>+	nla_overwrite = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];

Just a nitpick, better to name this "nla_overwrite_mask" to follow the
name of the netlink attr.

Otherwise (extept the uapi BIT as Jakub pointed out) this looks fine to
me.


>+	if (nla_overwrite) {
>+		struct nla_bitfield32 sections;
>+
>+		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK)) {
>+			NL_SET_ERR_MSG_ATTR(info->extack, nla_overwrite,
>+					    "overwrite is not supported");
>+			return -EOPNOTSUPP;
>+		}
>+		sections = nla_get_bitfield32(nla_overwrite);
>+		params.overwrite_mask = sections.value & sections.selector;
>+	}
>+
> 	return devlink->ops->flash_update(devlink, &params, info->extack);
> }
> 
>@@ -7046,6 +7059,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
> 	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
>+	[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK] =
>+		NLA_POLICY_BITFIELD32(DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS),
> 	[DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_TRAP_ACTION] = { .type = NLA_U8 },
> 	[DEVLINK_ATTR_TRAP_GROUP_NAME] = { .type = NLA_NUL_STRING },
>-- 
>2.28.0.218.ge27853923b9d.dirty
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ