| 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: <c316b1be-d18f-4bb0-8434-bcc9236619df@t-8ch.de>
Date: Tue, 21 Jan 2025 14:29:17 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
eric.snowberg@...cle.com, corbet@....net, mcgrof@...nel.org, petr.pavlu@...e.com,
samitolvanen@...gle.com, da.gomez@...sung.com, akpm@...ux-foundation.org,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com, shuah@...nel.org,
mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, linux-integrity@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
linux-modules@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kselftest@...r.kernel.org, wufan@...ux.microsoft.com, pbrobinson@...il.com,
zbyszek@...waw.pl, hch@....de, mjg59@...f.ucam.org, pmatilai@...hat.com,
jannh@...gle.com, dhowells@...hat.com, jikos@...nel.org, mkoutny@...e.com,
ppavlu@...e.com, petr.vorel@...il.com, mzerqung@...inter.de, kgold@...ux.ibm.com,
Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v6 01/15] lib: Add TLV parser
Hi Robert,
On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@...wei.com>
>
> Add a parser of a generic Type-Length-Value (TLV) format:
>
> +--------------+--+---------+--------+---------+
> | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> +--------------+------------+------------------+
> | ... | ... | ... |
> +--------------+------------+------------------+
> | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> +--------------+------------+------------------+
Should mention that its big endian.
> Each adopter can define its own fields. The TLV parser does not need to be
> aware of those, but lets the adopter obtain the data and decide how to
"adopter" -> "user".
> continue.
>
> After processing a TLV entry, call the callback function also with the
> callback data provided by the adopter. The latter can decide how to
> interpret the TLV entry depending on the field ID.
>
> Nesting TLVs is also possible, the callback function can call tlv_parse()
> to parse the inner structure.
Given that we already have the netlink data structures, helpers and
infrastructure, what is the advantage over those?
>
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> ---
> MAINTAINERS | 8 +++
> include/linux/tlv_parser.h | 32 ++++++++++++
> include/uapi/linux/tlv_parser.h | 41 ++++++++++++++++
> lib/Kconfig | 3 ++
> lib/Makefile | 2 +
> lib/tlv_parser.c | 87 +++++++++++++++++++++++++++++++++
> lib/tlv_parser.h | 18 +++++++
> 7 files changed, 191 insertions(+)
> create mode 100644 include/linux/tlv_parser.h
> create mode 100644 include/uapi/linux/tlv_parser.h
> create mode 100644 lib/tlv_parser.c
> create mode 100644 lib/tlv_parser.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a097afd76ded..1f7ffa1c9dbd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23388,6 +23388,14 @@ W: http://sourceforge.net/projects/tlan/
> F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> F: drivers/net/ethernet/ti/tlan.*
>
> +TLV PARSER
> +M: Roberto Sassu <roberto.sassu@...wei.com>
> +L: linux-kernel@...r.kernel.org
> +S: Maintained
> +F: include/linux/tlv_parser.h
> +F: include/uapi/linux/tlv_parser.h
> +F: lib/tlv_parser.*
> +
> TMIO/SDHI MMC DRIVER
> M: Wolfram Sang <wsa+renesas@...g-engineering.com>
> L: linux-mmc@...r.kernel.org
> diff --git a/include/linux/tlv_parser.h b/include/linux/tlv_parser.h
> new file mode 100644
> index 000000000000..0c72742af548
> --- /dev/null
> +++ b/include/linux/tlv_parser.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> + *
> + * Header file of TLV parser.
> + */
> +
> +#ifndef _LINUX_TLV_PARSER_H
> +#define _LINUX_TLV_PARSER_H
> +
> +#include <uapi/linux/tlv_parser.h>
> +
> +/**
> + * typedef callback - Callback after parsing TLV entry
> + * @callback_data: Opaque data to supply to the callback function
> + * @field: Field identifier
> + * @field_data: Field data
> + * @field_len: Length of @field_data
> + *
> + * This callback is invoked after a TLV entry is parsed.
> + *
> + * Return: Zero on success, a negative value on error.
It's not explained what happens on error.
> + */
> +typedef int (*callback)(void *callback_data, __u16 field,
> + const __u8 *field_data, __u32 field_len);
No need for __underscore types in kernel-only signatures.
> +
> +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> + size_t data_len, const char **fields, __u32 num_fields);
> +
> +#endif /* _LINUX_TLV_PARSER_H */
> diff --git a/include/uapi/linux/tlv_parser.h b/include/uapi/linux/tlv_parser.h
> new file mode 100644
> index 000000000000..171d0cfd2c4c
> --- /dev/null
> +++ b/include/uapi/linux/tlv_parser.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> + *
> + * Implement the user space interface for the TLV parser.
> + */
Can you explain in the commit message where this will be exposed to
userspace as binary?
> +
> +#ifndef _UAPI_LINUX_TLV_PARSER_H
> +#define _UAPI_LINUX_TLV_PARSER_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * TLV format:
> + *
> + * +--------------+--+---------+--------+---------+
> + * | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> + * +--------------+------------+------------------+
> + * | ... | ... | ... |
> + * +--------------+------------+------------------+
> + * | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> + * +--------------+------------+------------------+
> + */
> +
> +/**
> + * struct tlv_entry - Entry of TLV format
> + * @field: Field identifier
> + * @length: Data length
> + * @data: Data
> + *
> + * This structure represents an entry of the TLV format.
> + */
> +struct tlv_entry {
> + __u16 field;
> + __u32 length;
Use __be16 and __be32 here.
> + __u8 data[];
__counted_by()?
Not sure how this interacts with __be.
> +} __attribute__((packed));
> +
> +#endif /* _UAPI_LINUX_TLV_PARSER_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index b38849af6f13..9141dcfc1704 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -777,3 +777,6 @@ config POLYNOMIAL
>
> config FIRMWARE_TABLE
> bool
> +
> +config TLV_PARSER
> + bool
> diff --git a/lib/Makefile b/lib/Makefile
> index 773adf88af41..630de494eab5 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -393,5 +393,7 @@ obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>
> obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
> +obj-$(CONFIG_TLV_PARSER) += tlv_parser.o
> +CFLAGS_tlv_parser.o += -I lib
Does this work with out of tree builds?
>
> subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> diff --git a/lib/tlv_parser.c b/lib/tlv_parser.c
> new file mode 100644
> index 000000000000..dbbe08018b4d
> --- /dev/null
> +++ b/lib/tlv_parser.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> + *
> + * Implement the TLV parser.
> + */
> +
> +#define pr_fmt(fmt) "tlv_parser: "fmt
> +#include <tlv_parser.h>
This should be "tlv_parser.h",
but the header files looks unnecessary in the first place.
> +
> +/**
> + * tlv_parse - Parse TLV data
> + * @callback: Callback function to call to parse the entries
> + * @callback_data: Opaque data to supply to the callback function
> + * @data: Data to parse
> + * @data_len: Length of @data
> + * @fields: Array of field strings
> + * @num_fields: Number of elements of @fields
> + *
> + * Parse the TLV data format and call the supplied callback function for each
> + * entry, passing also the opaque data pointer.
> + *
> + * The callback function decides how to process data depending on the field.
Mention that a callback return an error will abort the whole parsing.
> + *
> + * Return: Zero on success, a negative value on error.
> + */
> +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> + size_t data_len, const char **fields, __u32 num_fields)
No need for __underscore types in kernel-only functions.
"num_fields" and "fields" are accessed without checking for validity.
"fields" is only every used for debug logging, so can be removed.
"num_fields" probably, too.
> +{
> + const __u8 *data_ptr = data;
> + struct tlv_entry *entry;
This comes from the input data, should also be const.
> + __u16 parsed_field;
> + __u32 len;
field_len
> + int ret;
> +
> + if (data_len > U32_MAX) {
> + pr_debug("Data too big, size: %zd\n", data_len);
> + return -E2BIG;
> + }
> +
> + while (data_len) {
> + if (data_len < sizeof(*entry))
> + return -EBADMSG;
> +
> + entry = (struct tlv_entry *)data_ptr;
> + data_ptr += sizeof(*entry);
> + data_len -= sizeof(*entry);
> +
> + parsed_field = __be16_to_cpu(entry->field);
This doesn't seem to handle invalid alignment, some architectures will
trap unaligned accesses.
Depending on the size and usage patterns it may make sense to document
some alignment recommendations/requirements.
(Not sure how big of a performance difference it would make)
> + if (parsed_field >= num_fields) {
> + pr_debug("Invalid field %u, max: %u\n",
> + parsed_field, num_fields - 1);
> + return -EBADMSG;
> + }
> +
> + len = __be32_to_cpu(entry->length);
> +
> + if (data_len < len)
> + return -EBADMSG;
> +
> + pr_debug("Data: field: %s, len: %u\n", fields[parsed_field],
> + len);
> +
> + if (!len)
> + continue;
Empty fields are discarded silently, is this intentional?
It should be documented. Those fields could be useful for flag data.
> +
> + ret = callback(callback_data, parsed_field, data_ptr, len);
> + if (ret < 0) {
> + pr_debug("Parsing of field %s failed, ret: %d\n",
> + fields[parsed_field], ret);
> + return ret;
> + }
> +
> + data_ptr += len;
> + data_len -= len;
> + }
> +
> + if (data_len) {
Can this ever happen?
The check at the beginning of the loop should have caught it already.
> + pr_debug("Excess data: %zu bytes\n", data_len);
> + return -EBADMSG;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tlv_parse);
Some kunit tests would be great.
> diff --git a/lib/tlv_parser.h b/lib/tlv_parser.h
> new file mode 100644
> index 000000000000..e663966deac5
> --- /dev/null
> +++ b/lib/tlv_parser.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@...wei.com>
> + *
> + * Header file of TLV parser.
> + */
> +
> +#ifndef _LIB_TLV_PARSER_H
> +#define _LIB_TLV_PARSER_H
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/limits.h>
> +#include <linux/tlv_parser.h>
The #includes should move to the .c file and the header be removed.
> +
> +#endif /* _LIB_TLV_PARSER_H */
> --
> 2.47.0.118.gfd3785337b
>
Powered by blists - more mailing lists