diff mbox series

[RFC,04/12] digest_lists: Objects

Message ID 20210625165614.2284243-5-roberto.sassu@huawei.com
State New
Headers show
Series None | expand

Commit Message

Roberto Sassu June 25, 2021, 4:56 p.m. UTC
This patch defines the objects to manage digest lists:

- digest_list_item: represents a digest list;
- digest_list_item_ref: represents a reference to a digest list, i.e. the
  location at which a digest within a digest list can be accessed;
- digest_item: represents a unique digest.

It also defines some helpers for the objects. More information can be found
in Documentation/security/digest_lists.rst.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 Documentation/security/digest_lists.rst       | 156 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 .../integrity/digest_lists/digest_lists.h     | 117 +++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 security/integrity/digest_lists/digest_lists.h

Comments

Greg Kroah-Hartman June 27, 2021, 10:56 a.m. UTC | #1
On Fri, Jun 25, 2021 at 06:56:06PM +0200, Roberto Sassu wrote:
> +++ b/security/integrity/digest_lists/digest_lists.h

> @@ -0,0 +1,117 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Copyright (C) 2005,2006,2007,2008 IBM Corporation

> + * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH

> + *

> + * Author: Roberto Sassu <roberto.sassu@huawei.com>

> + *

> + * This program is free software; you can redistribute it and/or

> + * modify it under the terms of the GNU General Public License as

> + * published by the Free Software Foundation, version 2 of the

> + * License.

> + *

> + * File: digest_lists.h

> + *      Unexported definitions for digest lists.


Unexported to whom?

> + */

> +

> +#ifndef __DIGEST_LISTS_INTERNAL_H

> +#define __DIGEST_LISTS_INTERNAL_H

> +

> +#include <linux/types.h>

> +#include <linux/crypto.h>

> +#include <linux/fs.h>

> +#include <linux/security.h>

> +#include <linux/hash.h>

> +#include <linux/tpm.h>

> +#include <linux/audit.h>

> +#include <crypto/hash_info.h>

> +#include <linux/hash_info.h>

> +#include <uapi/linux/digest_lists.h>

> +

> +#define MAX_DIGEST_SIZE	64

> +#define HASH_BITS 10

> +#define MEASURE_HTABLE_SIZE (1 << HASH_BITS)

> +

> +struct digest_list_item {

> +	loff_t size;

> +	u8 *buf;

> +	u8 actions;

> +	u8 digest[64];

> +	enum hash_algo algo;

> +	const char *label;

> +};

> +

> +struct digest_list_item_ref {

> +	struct digest_list_item *digest_list;

> +	loff_t digest_offset;

> +	loff_t hdr_offset;

> +};

> +

> +struct digest_item {

> +	/* hash table pointers */

> +	struct hlist_node hnext;

> +	/* digest list references (protected by RCU) */

> +	struct digest_list_item_ref *refs;

> +};

> +

> +struct h_table {

> +	atomic_long_t len;


Why is this atomic?  Why would that matter?

> +	struct hlist_head queue[MEASURE_HTABLE_SIZE];

> +};

> +

> +static inline unsigned int hash_key(u8 *digest)

> +{

> +	return (digest[0] | digest[1] << 8) % MEASURE_HTABLE_SIZE;

> +}


Don't we have hashing functions in the kernel already?

> +

> +static inline struct compact_list_hdr *get_hdr(

> +					struct digest_list_item *digest_list,

> +					loff_t hdr_offset)

> +{

> +	return (struct compact_list_hdr *)(digest_list->buf + hdr_offset);

> +}


pointer math feels rough, are you shure you want to do this this way?

thanks,

greg k-h
Roberto Sassu June 28, 2021, 8:14 a.m. UTC | #2
> From: Greg KH [mailto:gregkh@linuxfoundation.org]

> Sent: Sunday, June 27, 2021 12:56 PM

> On Fri, Jun 25, 2021 at 06:56:06PM +0200, Roberto Sassu wrote:

> > +++ b/security/integrity/digest_lists/digest_lists.h

> > @@ -0,0 +1,117 @@

> > +/* SPDX-License-Identifier: GPL-2.0 */

> > +/*

> > + * Copyright (C) 2005,2006,2007,2008 IBM Corporation

> > + * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH

> > + *

> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>

> > + *

> > + * This program is free software; you can redistribute it and/or

> > + * modify it under the terms of the GNU General Public License as

> > + * published by the Free Software Foundation, version 2 of the

> > + * License.

> > + *

> > + * File: digest_lists.h

> > + *      Unexported definitions for digest lists.

> 

> Unexported to whom?


Hi Greg

I meant not placed in include/linux.

> > + */

> > +

> > +#ifndef __DIGEST_LISTS_INTERNAL_H

> > +#define __DIGEST_LISTS_INTERNAL_H

> > +

> > +#include <linux/types.h>

> > +#include <linux/crypto.h>

> > +#include <linux/fs.h>

> > +#include <linux/security.h>

> > +#include <linux/hash.h>

> > +#include <linux/tpm.h>

> > +#include <linux/audit.h>

> > +#include <crypto/hash_info.h>

> > +#include <linux/hash_info.h>

> > +#include <uapi/linux/digest_lists.h>

> > +

> > +#define MAX_DIGEST_SIZE	64

> > +#define HASH_BITS 10

> > +#define MEASURE_HTABLE_SIZE (1 << HASH_BITS)

> > +

> > +struct digest_list_item {

> > +	loff_t size;

> > +	u8 *buf;

> > +	u8 actions;

> > +	u8 digest[64];

> > +	enum hash_algo algo;

> > +	const char *label;

> > +};

> > +

> > +struct digest_list_item_ref {

> > +	struct digest_list_item *digest_list;

> > +	loff_t digest_offset;

> > +	loff_t hdr_offset;

> > +};

> > +

> > +struct digest_item {

> > +	/* hash table pointers */

> > +	struct hlist_node hnext;

> > +	/* digest list references (protected by RCU) */

> > +	struct digest_list_item_ref *refs;

> > +};

> > +

> > +struct h_table {

> > +	atomic_long_t len;

> 

> Why is this atomic?  Why would that matter?


Yes, it shouldn't be. There are not concurrent updates.

> > +	struct hlist_head queue[MEASURE_HTABLE_SIZE];

> > +};

> > +

> > +static inline unsigned int hash_key(u8 *digest)

> > +{

> > +	return (digest[0] | digest[1] << 8) % MEASURE_HTABLE_SIZE;

> > +}

> 

> Don't we have hashing functions in the kernel already?


We had a discussion before:

https://lore.kernel.org/linux-integrity/1587739544.5190.14.camel@linux.ibm.com/

It seems there is no real advantage in hashing a digest.

> > +

> > +static inline struct compact_list_hdr *get_hdr(

> > +					struct digest_list_item *digest_list,

> > +					loff_t hdr_offset)

> > +{

> > +	return (struct compact_list_hdr *)(digest_list->buf + hdr_offset);

> > +}

> 

> pointer math feels rough, are you shure you want to do this this way?


Maybe, I could change digest_list_item_ref to:

struct digest_list_item_ref {
	struct digest_list_item *digest_list;
	u8 *digest;
	struct compact_list_hdr *hdr;
};

where digest and hdr are calculated in the same way.

Or you have a different suggestion?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> thanks,

> 

> greg k-h
Greg Kroah-Hartman June 28, 2021, 8:47 a.m. UTC | #3
On Mon, Jun 28, 2021 at 08:14:41AM +0000, Roberto Sassu wrote:
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]

> > Sent: Sunday, June 27, 2021 12:56 PM

> > On Fri, Jun 25, 2021 at 06:56:06PM +0200, Roberto Sassu wrote:

> > > +++ b/security/integrity/digest_lists/digest_lists.h

> > > @@ -0,0 +1,117 @@

> > > +/* SPDX-License-Identifier: GPL-2.0 */

> > > +/*

> > > + * Copyright (C) 2005,2006,2007,2008 IBM Corporation

> > > + * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH

> > > + *

> > > + * Author: Roberto Sassu <roberto.sassu@huawei.com>

> > > + *

> > > + * This program is free software; you can redistribute it and/or

> > > + * modify it under the terms of the GNU General Public License as

> > > + * published by the Free Software Foundation, version 2 of the

> > > + * License.

> > > + *

> > > + * File: digest_lists.h

> > > + *      Unexported definitions for digest lists.

> > 

> > Unexported to whom?

> 

> Hi Greg

> 

> I meant not placed in include/linux.


That's obvious based on the location of the file :)

> > > +

> > > +static inline struct compact_list_hdr *get_hdr(

> > > +					struct digest_list_item *digest_list,

> > > +					loff_t hdr_offset)

> > > +{

> > > +	return (struct compact_list_hdr *)(digest_list->buf + hdr_offset);

> > > +}

> > 

> > pointer math feels rough, are you shure you want to do this this way?

> 

> Maybe, I could change digest_list_item_ref to:

> 

> struct digest_list_item_ref {

> 	struct digest_list_item *digest_list;

> 	u8 *digest;

> 	struct compact_list_hdr *hdr;

> };

> 

> where digest and hdr are calculated in the same way.


That works better, no need to do pointer math if you do not have to.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/Documentation/security/digest_lists.rst b/Documentation/security/digest_lists.rst
index 995260294783..1031667324c9 100644
--- a/Documentation/security/digest_lists.rst
+++ b/Documentation/security/digest_lists.rst
@@ -345,3 +345,159 @@  with digest lists:
 
 - ``DIGEST_LIST_ADD``: the digest list is being added;
 - ``DIGEST_LIST_DEL``: the digest list is being deleted.
+
+
+Objects
+-------
+
+This section defines the objects to manage digest lists:
+
+- ``digest_list_item``: represents a digest list;
+- ``digest_list_item_ref``: represents a reference to a digest list,
+  i.e. the location at which a digest within a digest list can be accessed;
+- ``digest_item``: represents a unique digest.
+
+They are represented in the following class diagram:
+
+::
+
+ digest_offset,-----------+
+ hdr_offset               |
+                          |
+ +------------------+     |     +----------------------+
+ | digest_list_item |--- N:1 ---| digest_list_item_ref |
+ +------------------+           +----------------------+
+                                           |
+                                          1:N
+                                           |
+                                    +-------------+
+                                    | digest_item |
+                                    +-------------+
+
+A ``digest_list_item`` is associated to one or multiple
+``digest_list_item_ref``, one for each digest it contains. However,
+a ``digest_list_item_ref`` is associated to only one ``digest_list_item``,
+as it represents a single location within a specific digest list.
+
+Given that a ``digest_list_item_ref`` represents a single location, it is
+associated to only one ``digest_item``. However, a ``digest_item`` can have
+multiple references (as it might appears multiple times within the same
+digest list or in different digest lists, if it is duplicated).
+
+
+A ``digest_list_item`` is defined as:
+
+::
+
+	struct digest_list_item {
+		loff_t size;
+		u8 *buf;
+		u8 actions;
+		u8 digest[64];
+		enum hash_algo algo;
+		const char *label;
+	};
+
+- ``size``: size of the digest list buffer;
+- ``buf``: digest list buffer;
+- ``actions``: actions performed on the digest list;
+- ``digest``: digest of the digest list;
+- ``algo``: digest algorithm;
+- ``label``: label used to identify the digest list (e.g. file name).
+
+A ``digest_list_item_ref`` is defined as:
+
+::
+
+	struct digest_list_item_ref {
+		struct digest_list_item *digest_list;
+		loff_t digest_offset;
+		loff_t hdr_offset;
+	};
+
+- ``digest_list``: pointer to a ``digest_list_item`` structure;
+- ``digest_offset``: offset of the digest related to the digest list
+  buffer;
+- ``hdr_offset``: offset of the header of the digest block containing the
+  digest.
+
+A ``digest_item`` is defined as:
+
+::
+
+	struct digest_item {
+		struct hlist_node hnext;
+		struct digest_list_item_ref *refs;
+	};
+
+- ``hnext``: pointers of the hash table;
+- ``refs``: array of ``digest_list_item_ref`` structures including a
+  terminator (protected by RCU).
+
+All digest list references are stored for a given digest, so that a query
+result can include the OR of the modifiers and actions of each referenced
+digest list.
+
+The relationship between the described objects can be graphically
+represented as:
+
+::
+
+ Hash table            +-------------+         +-------------+
+ PARSER      +-----+   | digest_item |         | digest_item |
+ FILE        | key |-->|             |-->...-->|             |
+ METADATA    +-----+   |ref0|...|refN|         |ref0|...|refN|
+                       +-------------+         +-------------+
+            ref0:         |                               | refN:
+            digest_offset | +-----------------------------+ digest_offset
+            hdr_offset    | |                               hdr_offset
+                          V V
+                     +--------------------+
+                     |  digest_list_item  |
+                     |                    |
+                     | size, buf, actions |
+                     +--------------------+
+                          ^
+                          |
+ Hash table            +-------------+         +-------------+
+ DIGEST_LIST +-----+   |ref0         |         |ref0         |
+             | key |-->|             |-->...-->|             |
+             +-----+   | digest_item |         | digest_item |
+                       +-------------+         +-------------+
+
+The reference for the digest of the digest list differs from the references
+for the other digest types. ``digest_offset`` and ``hdr_offset`` are set to
+zero, so that the digest of the digest list is retrieved from the
+``digest_list_item`` structure directly (see ``get_digest()`` below).
+
+Finally, this section defines useful helpers to access a digest or the
+header the digest belongs to. For example:
+
+::
+
+ static inline struct compact_list_hdr *get_hdr(
+                                      struct digest_list_item *digest_list,
+                                      loff_t hdr_offset)
+ {
+         return (struct compact_list_hdr *)(digest_list->buf + hdr_offset);
+ }
+
+the header can be obtained by summing the address of the digest list buffer
+in the ``digest_list_item`` structure with ``hdr_offset``.
+
+Similarly:
+
+::
+
+ static inline u8 *get_digest(struct digest_list_item *digest_list,
+                              loff_t digest_offset, loff_t hdr_offset)
+ {
+         /* Digest list digest is stored in a different place. */
+         if (!digest_offset)
+                 return digest_list->digest;
+         return digest_list->buf + digest_offset;
+ }
+
+the digest can be obtained by summing the address of the digest list buffer
+with ``digest_offset`` (except for the digest lists, where the digest is
+stored in the ``digest`` field of the ``digest_list_item`` structure).
diff --git a/MAINTAINERS b/MAINTAINERS
index ccf555862673..9a7e9f16eee8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8387,6 +8387,7 @@  L:	linux-integrity@vger.kernel.org
 S:	Supported
 T:	git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
 F:	Documentation/security/digest_lists.rst
+F:	security/integrity/digest_lists/digest_list.h
 F:	uapi/linux/digest_lists.h
 
 HUAWEI ETHERNET DRIVER
diff --git a/security/integrity/digest_lists/digest_lists.h b/security/integrity/digest_lists/digest_lists.h
new file mode 100644
index 000000000000..81b6cb10f4f1
--- /dev/null
+++ b/security/integrity/digest_lists/digest_lists.h
@@ -0,0 +1,117 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: digest_lists.h
+ *      Unexported definitions for digest lists.
+ */
+
+#ifndef __DIGEST_LISTS_INTERNAL_H
+#define __DIGEST_LISTS_INTERNAL_H
+
+#include <linux/types.h>
+#include <linux/crypto.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/hash.h>
+#include <linux/tpm.h>
+#include <linux/audit.h>
+#include <crypto/hash_info.h>
+#include <linux/hash_info.h>
+#include <uapi/linux/digest_lists.h>
+
+#define MAX_DIGEST_SIZE	64
+#define HASH_BITS 10
+#define MEASURE_HTABLE_SIZE (1 << HASH_BITS)
+
+struct digest_list_item {
+	loff_t size;
+	u8 *buf;
+	u8 actions;
+	u8 digest[64];
+	enum hash_algo algo;
+	const char *label;
+};
+
+struct digest_list_item_ref {
+	struct digest_list_item *digest_list;
+	loff_t digest_offset;
+	loff_t hdr_offset;
+};
+
+struct digest_item {
+	/* hash table pointers */
+	struct hlist_node hnext;
+	/* digest list references (protected by RCU) */
+	struct digest_list_item_ref *refs;
+};
+
+struct h_table {
+	atomic_long_t len;
+	struct hlist_head queue[MEASURE_HTABLE_SIZE];
+};
+
+static inline unsigned int hash_key(u8 *digest)
+{
+	return (digest[0] | digest[1] << 8) % MEASURE_HTABLE_SIZE;
+}
+
+static inline struct compact_list_hdr *get_hdr(
+					struct digest_list_item *digest_list,
+					loff_t hdr_offset)
+{
+	return (struct compact_list_hdr *)(digest_list->buf + hdr_offset);
+}
+
+static inline enum hash_algo get_algo(struct digest_list_item *digest_list,
+				      loff_t digest_offset, loff_t hdr_offset)
+{
+	/* Digest list digest algorithm is stored in a different place. */
+	if (!digest_offset)
+		return digest_list->algo;
+
+	return get_hdr(digest_list, hdr_offset)->algo;
+}
+
+static inline u8 *get_digest(struct digest_list_item *digest_list,
+			     loff_t digest_offset, loff_t hdr_offset)
+{
+	/* Digest list digest is stored in a different place. */
+	if (!digest_offset)
+		return digest_list->digest;
+
+	return digest_list->buf + digest_offset;
+}
+
+static inline struct compact_list_hdr *get_hdr_ref(
+					struct digest_list_item_ref *ref)
+{
+	return get_hdr(ref->digest_list, ref->hdr_offset);
+}
+
+static inline enum hash_algo get_algo_ref(struct digest_list_item_ref *ref)
+{
+	/* Digest list digest algorithm is stored in a different place. */
+	if (!ref->digest_offset)
+		return ref->digest_list->algo;
+
+	return get_hdr_ref(ref)->algo;
+}
+
+static inline u8 *get_digest_ref(struct digest_list_item_ref *ref)
+{
+	/* Digest list digest is stored in a different place. */
+	if (!ref->digest_offset)
+		return ref->digest_list->digest;
+
+	return ref->digest_list->buf + ref->digest_offset;
+}
+#endif /*__DIGEST_LISTS_INTERNAL_H*/