[PATCH 0/1] ubifs: support authentication without hmac

[PATCH 0/1] ubifs: support authentication without hmac

[Torben Hohn]

This patch adds support for ubifs authentication without HMAC,
which obviously only works for a read-only mount.

ubiblock and dm-verity are not supported by u-boot, and
the kernel on the target is loaded by u-boot out of the RFS.

This is a first try to implement this.
It boots fine, and the WARN_ON is not triggered.

I plan to update the docs also, but i would like to have
some positive comments on this before.


Torben Hohn (1):
  ubifs: support authentication, for ro mount, when no key is given

 fs/ubifs/auth.c    | 69 ++++++++++++++++++++++++++++++++++++++++++----
 fs/ubifs/gc.c      |  2 +-
 fs/ubifs/journal.c | 12 ++++----
 fs/ubifs/lpt.c     |  4 +--
 fs/ubifs/master.c  |  2 +-
 fs/ubifs/replay.c  |  2 +-
 fs/ubifs/sb.c      | 16 +++++++----
 fs/ubifs/super.c   | 21 ++++++++++----
 fs/ubifs/ubifs.h   | 48 +++++++++++++++++++++-----------
 9 files changed, 133 insertions(+), 43 deletions(-)

-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Torben Hohn]

Ubifs authentication requires a hmac key, even when a
filesystem is mounted read-only.

Split c->authenticated into _rw and _ro.

Also implement ubifs_init_authentication_read_only(),
which only allocates the structures needed for validating
the hashes. That is called, when no auth_key_name is specified,
only auth_hash_name. It sets c->authenticated_ro to true.

Then implement ubifs_authenticated_read() and
ubifs_authenticated_write(). And change all occurences of
ubifs_authenticated() to the respective read or write version.

ubifs_authenticated_write() verifies, that it is not called
when only c->authenticated_ro is active. (The WARN_ON should
probably be BUG_ON(), though)

Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
---
 fs/ubifs/auth.c    | 69 ++++++++++++++++++++++++++++++++++++++++++----
 fs/ubifs/gc.c      |  2 +-
 fs/ubifs/journal.c | 12 ++++----
 fs/ubifs/lpt.c     |  4 +--
 fs/ubifs/master.c  |  2 +-
 fs/ubifs/replay.c  |  2 +-
 fs/ubifs/sb.c      | 16 +++++++----
 fs/ubifs/super.c   | 21 ++++++++++----
 fs/ubifs/ubifs.h   | 48 +++++++++++++++++++++-----------
 9 files changed, 133 insertions(+), 43 deletions(-)

diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c
index cc5c0abfd536..5429a05f537e 100644
--- a/fs/ubifs/auth.c
+++ b/fs/ubifs/auth.c
@@ -94,7 +94,7 @@ static struct shash_desc *ubifs_get_desc(const struct ubifs_info *c,
 	struct shash_desc *desc;
 	int err;
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_read(c))
 		return NULL;
 
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(tfm), GFP_KERNEL);
@@ -248,6 +248,61 @@ int ubifs_sb_verify_signature(struct ubifs_info *c,
 	return err;
 }
 
+/**
+ * ubifs_init_authentication_read_only - init only the read_only parts
+ *
+ * @c: UBIFS file-system description object
+ *
+ * This function returns 0 for success or a negative error code otherwise.
+ */
+
+int ubifs_init_authentication_read_only(struct ubifs_info *c)
+{
+	int err;
+
+	if (!c->auth_hash_name) {
+		ubifs_err(c, "authentication hash name needed with authentication");
+		return -EINVAL;
+	}
+
+	c->auth_hash_algo = match_string(hash_algo_name, HASH_ALGO__LAST,
+					 c->auth_hash_name);
+	if ((int)c->auth_hash_algo < 0) {
+		ubifs_err(c, "Unknown hash algo %s specified",
+			  c->auth_hash_name);
+		return -EINVAL;
+	}
+
+	c->hash_tfm = crypto_alloc_shash(c->auth_hash_name, 0, 0);
+	if (IS_ERR(c->hash_tfm)) {
+		err = PTR_ERR(c->hash_tfm);
+		ubifs_err(c, "Can not allocate %s: %d",
+			  c->auth_hash_name, err);
+		goto out;
+	}
+
+	c->hash_len = crypto_shash_digestsize(c->hash_tfm);
+	if (c->hash_len > UBIFS_HASH_ARR_SZ) {
+		ubifs_err(c, "hash %s is bigger than maximum allowed hash size (%d > %d)",
+			  c->auth_hash_name, c->hash_len, UBIFS_HASH_ARR_SZ);
+		err = -EINVAL;
+		goto out_free_hash;
+	}
+
+	c->authenticated_ro = true;
+
+	c->log_hash = ubifs_hash_get_desc(c);
+	if (IS_ERR(c->log_hash))
+		goto out_free_hash;
+
+	err = 0;
+out_free_hash:
+	if (err)
+		crypto_free_shash(c->hash_tfm);
+out:
+	return err;
+}
+
 /**
  * ubifs_init_authentication - initialize UBIFS authentication support
  * @c: UBIFS file-system description object
@@ -335,7 +390,7 @@ int ubifs_init_authentication(struct ubifs_info *c)
 	if (err)
 		goto out_free_hmac;
 
-	c->authenticated = true;
+	c->authenticated_rw = true;
 
 	c->log_hash = ubifs_hash_get_desc(c);
 	if (IS_ERR(c->log_hash))
@@ -364,11 +419,15 @@ int ubifs_init_authentication(struct ubifs_info *c)
  */
 void __ubifs_exit_authentication(struct ubifs_info *c)
 {
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_read(c))
 		return;
 
-	crypto_free_shash(c->hmac_tfm);
 	crypto_free_shash(c->hash_tfm);
+
+	if (!c->authenticated_rw)
+		return;
+
+	crypto_free_shash(c->hmac_tfm);
 	kfree(c->log_hash);
 }
 
@@ -511,7 +570,7 @@ int ubifs_hmac_wkm(struct ubifs_info *c, u8 *hmac)
 	int err;
 	const char well_known_message[] = "UBIFS";
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_write(c))
 		return 0;
 
 	shash->tfm = c->hmac_tfm;
diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
index 62cb3db44e6e..8616e8ba97d6 100644
--- a/fs/ubifs/gc.c
+++ b/fs/ubifs/gc.c
@@ -410,7 +410,7 @@ static int move_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb)
 			moved = 1;
 		}
 
-		if (ubifs_authenticated(c) && moved) {
+		if (ubifs_authenticated_write(c) && moved) {
 			struct ubifs_auth_node *auth;
 
 			auth = kmalloc(ubifs_auth_node_sz(c), GFP_NOFS);
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index e5ec1afe1c66..da13e5eb93e1 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -80,7 +80,7 @@ static inline void zero_trun_node_unused(struct ubifs_trun_node *trun)
 
 static void ubifs_add_auth_dirt(struct ubifs_info *c, int lnum)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c));
 }
 
@@ -278,7 +278,7 @@ static int write_head(struct ubifs_info *c, int jhead, void *buf, int len,
 	dbg_jnl("jhead %s, LEB %d:%d, len %d",
 		dbg_jhead(jhead), *lnum, *offs, len);
 
-	if (ubifs_authenticated(c)) {
+	if (ubifs_authenticated_write(c)) {
 		err = ubifs_hash_nodes(c, buf, len, c->jheads[jhead].log_hash);
 		if (err)
 			return err;
@@ -572,7 +572,7 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 
 	len = aligned_dlen + aligned_ilen + UBIFS_INO_NODE_SZ;
 	/* Make sure to also account for extended attributes */
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		len += ALIGN(host_ui->data_len, 8) + ubifs_auth_node_sz(c);
 	else
 		len += host_ui->data_len;
@@ -778,7 +778,7 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
 	}
 
 	dlen = UBIFS_DATA_NODE_SZ + out_len;
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		write_len = ALIGN(dlen, 8) + auth_len;
 	else
 		write_len = dlen;
@@ -860,7 +860,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode)
 		write_len += UBIFS_INO_NODE_SZ * ui->xattr_cnt;
 	}
 
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		write_len += ALIGN(ilen, 8) + ubifs_auth_node_sz(c);
 	else
 		write_len += ilen;
@@ -1572,7 +1572,7 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode,
 	/* Must make reservation before allocating sequence numbers */
 	len = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ;
 
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		len += ALIGN(dlen, 8) + ubifs_auth_node_sz(c);
 	else
 		len += dlen;
diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index e21abf250951..f3a7136518d1 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -1660,7 +1660,7 @@ int ubifs_lpt_calc_hash(struct ubifs_info *c, u8 *hash)
 	void *buf;
 	int err;
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_read(c))
 		return 0;
 
 	if (!c->nroot) {
@@ -1750,7 +1750,7 @@ static int lpt_check_hash(struct ubifs_info *c)
 	int err;
 	u8 hash[UBIFS_HASH_ARR_SZ];
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_read(c))
 		return 0;
 
 	err = ubifs_lpt_calc_hash(c, hash);
diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c
index 911d0555b9f2..57304e3f7016 100644
--- a/fs/ubifs/master.c
+++ b/fs/ubifs/master.c
@@ -129,7 +129,7 @@ static int scan_for_master(struct ubifs_info *c)
 	c->mst_offs = offs;
 	ubifs_scan_destroy(sleb);
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_read(c))
 		return 0;
 
 	if (ubifs_hmac_zero(c, c->mst_node->hmac)) {
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index b69ffac7e415..6c1b8739359c 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -595,7 +595,7 @@ static int authenticate_sleb(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 	u8 hash[UBIFS_HASH_ARR_SZ];
 	u8 hmac[UBIFS_HMAC_ARR_SZ];
 
-	if (!ubifs_authenticated(c))
+	if (!ubifs_authenticated_write(c))
 		return sleb->nodes_cnt;
 
 	list_for_each_entry(snod, &sleb->nodes, list) {
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 4b4b65b48c57..52396a92f8af 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -176,7 +176,7 @@ static int create_default_filesystem(struct ubifs_info *c)
 		sup_flags |= UBIFS_FLG_BIGLPT;
 	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
 
-	if (ubifs_authenticated(c)) {
+	if (ubifs_authenticated_write(c)) {
 		sup_flags |= UBIFS_FLG_AUTHENTICATION;
 		sup->hash_algo = cpu_to_le16(c->auth_hash_algo);
 		err = ubifs_hmac_wkm(c, sup->hmac_wkm);
@@ -542,20 +542,20 @@ static int authenticate_sb_node(struct ubifs_info *c,
 	int hash_algo;
 	int err;
 
-	if (c->authenticated && !authenticated) {
+	if (ubifs_authenticated_read(c) && !authenticated) {
 		ubifs_err(c, "authenticated FS forced, but found FS without authentication");
 		return -EINVAL;
 	}
 
-	if (!c->authenticated && authenticated) {
-		ubifs_err(c, "authenticated FS found, but no key given");
+	if (!ubifs_authenticated_read(c) && authenticated) {
+		ubifs_err(c, "authenticated FS found, but no key nor hash name given");
 		return -EINVAL;
 	}
 
 	ubifs_msg(c, "Mounting in %sauthenticated mode",
-		  c->authenticated ? "" : "un");
+		  c->authenticated_rw ? "RW " : (c->authenticated_ro ? "RO " :"un"));
 
-	if (!c->authenticated)
+	if (!ubifs_authenticated_read(c))
 		return 0;
 
 	if (!IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION))
@@ -583,6 +583,10 @@ static int authenticate_sb_node(struct ubifs_info *c,
 	if (ubifs_hmac_zero(c, sup->hmac)) {
 		err = ubifs_sb_verify_signature(c, sup);
 	} else {
+		if (!ubifs_authenticated_write(c)) {
+			ubifs_err(c, "HMAC authenticated FS found, but no key given");
+			return -EINVAL;
+		}
 		err = ubifs_hmac_wkm(c, hmac_wkm);
 		if (err)
 			return err;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7fc2f3f07c16..ec95f1f50e5e 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
 			err = -EINVAL;
 			goto out_free;
 		}
+	} else if (c->auth_hash_name) {
+		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
+			err = ubifs_init_authentication_read_only(c);
+			if (err)
+				goto out_free;
+		} else {
+			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
+				  " authentication support");
+			err = -EINVAL;
+			goto out_free;
+		}
 	}
 
 	err = ubifs_read_superblock(c);
@@ -1383,7 +1394,7 @@ static int mount_ubifs(struct ubifs_info *c)
 	 * in the superblock, we can update the offline signed
 	 * superblock with a HMAC version,
 	 */
-	if (ubifs_authenticated(c) && ubifs_hmac_zero(c, c->sup_node->hmac)) {
+	if (c->authenticated_rw && ubifs_hmac_zero(c, c->sup_node->hmac)) {
 		err = ubifs_hmac_wkm(c, c->sup_node->hmac_wkm);
 		if (err)
 			goto out_lpt;
@@ -1430,7 +1441,7 @@ static int mount_ubifs(struct ubifs_info *c)
 		}
 
 		if (c->need_recovery) {
-			if (!ubifs_authenticated(c)) {
+			if (!ubifs_authenticated_write(c)) {
 				err = ubifs_recover_size(c, true);
 				if (err)
 					goto out_orphans;
@@ -1440,7 +1451,7 @@ static int mount_ubifs(struct ubifs_info *c)
 			if (err)
 				goto out_orphans;
 
-			if (ubifs_authenticated(c)) {
+			if (ubifs_authenticated_write(c)) {
 				err = ubifs_recover_size(c, false);
 				if (err)
 					goto out_orphans;
@@ -1686,7 +1697,7 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 		err = ubifs_write_rcvrd_mst_node(c);
 		if (err)
 			goto out;
-		if (!ubifs_authenticated(c)) {
+		if (!ubifs_authenticated_write(c)) {
 			err = ubifs_recover_size(c, true);
 			if (err)
 				goto out;
@@ -1771,7 +1782,7 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 		if (err)
 			goto out;
 
-		if (ubifs_authenticated(c)) {
+		if (ubifs_authenticated_write(c)) {
 			err = ubifs_recover_size(c, false);
 			if (err)
 				goto out;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index bff682309fbe..0789c859a148 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1040,7 +1040,8 @@ struct ubifs_debug_info;
  * @default_compr: default compression algorithm (%UBIFS_COMPR_LZO, etc)
  * @rw_incompat: the media is not R/W compatible
  * @assert_action: action to take when a ubifs_assert() fails
- * @authenticated: flag indigating the FS is mounted in authenticated mode
+ * @authenticated_rw: flag indigating the FS is mounted in authenticated mode
+ * @authenticated_ro: flag indigating the FS is mounted in read-only authenticated mode (without HMAC)
  *
  * @tnc_mutex: protects the Tree Node Cache (TNC), @zroot, @cnext, @enext, and
  *             @calc_idx_sz
@@ -1293,7 +1294,8 @@ struct ubifs_info {
 	unsigned int default_compr:2;
 	unsigned int rw_incompat:1;
 	unsigned int assert_action:2;
-	unsigned int authenticated:1;
+	unsigned int authenticated_rw:1;
+	unsigned int authenticated_ro:1;
 	unsigned int superblock_need_write:1;
 
 	struct mutex tnc_mutex;
@@ -1506,21 +1508,34 @@ extern const struct inode_operations ubifs_symlink_inode_operations;
 extern struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
 
 /* auth.c */
-static inline int ubifs_authenticated(const struct ubifs_info *c)
+static inline int ubifs_authenticated_read(const struct ubifs_info *c)
 {
-	return (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) && c->authenticated;
+	return (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) && (c->authenticated_rw || c->authenticated_ro);
+}
+
+static inline int ubifs_authenticated_write(const struct ubifs_info *c)
+{
+	/*
+	 * Lets make sure, that ubifs does not try to write
+	 * when authenticate_ro is active.
+	 *
+	 * Because this will certeinly be an error.
+	 */
+	WARN_ON(c->authenticated_ro && !c->authenticated_rw);
+
+	return (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) && c->authenticated_rw;
 }
 
 struct shash_desc *__ubifs_hash_get_desc(const struct ubifs_info *c);
 static inline struct shash_desc *ubifs_hash_get_desc(const struct ubifs_info *c)
 {
-	return ubifs_authenticated(c) ? __ubifs_hash_get_desc(c) : NULL;
+	return ubifs_authenticated_read(c) ? __ubifs_hash_get_desc(c) : NULL;
 }
 
 static inline int ubifs_shash_init(const struct ubifs_info *c,
 				   struct shash_desc *desc)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_read(c))
 		return crypto_shash_init(desc);
 	else
 		return 0;
@@ -1532,7 +1547,7 @@ static inline int ubifs_shash_update(const struct ubifs_info *c,
 {
 	int err = 0;
 
-	if (ubifs_authenticated(c)) {
+	if (ubifs_authenticated_read(c)) {
 		err = crypto_shash_update(desc, buf, len);
 		if (err < 0)
 			return err;
@@ -1544,7 +1559,7 @@ static inline int ubifs_shash_update(const struct ubifs_info *c,
 static inline int ubifs_shash_final(const struct ubifs_info *c,
 				    struct shash_desc *desc, u8 *out)
 {
-	return ubifs_authenticated(c) ? crypto_shash_final(desc, out) : 0;
+	return ubifs_authenticated_read(c) ? crypto_shash_final(desc, out) : 0;
 }
 
 int __ubifs_node_calc_hash(const struct ubifs_info *c, const void *buf,
@@ -1552,7 +1567,7 @@ int __ubifs_node_calc_hash(const struct ubifs_info *c, const void *buf,
 static inline int ubifs_node_calc_hash(const struct ubifs_info *c,
 					const void *buf, u8 *hash)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_read(c))
 		return __ubifs_node_calc_hash(c, buf, hash);
 	else
 		return 0;
@@ -1599,17 +1614,18 @@ int __ubifs_node_check_hash(const struct ubifs_info *c, const void *buf,
 static inline int ubifs_node_check_hash(const struct ubifs_info *c,
 					const void *buf, const u8 *expected)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_read(c))
 		return __ubifs_node_check_hash(c, buf, expected);
 	else
 		return 0;
 }
 
+int ubifs_init_authentication_read_only(struct ubifs_info *c);
 int ubifs_init_authentication(struct ubifs_info *c);
 void __ubifs_exit_authentication(struct ubifs_info *c);
 static inline void ubifs_exit_authentication(struct ubifs_info *c)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_read(c))
 		__ubifs_exit_authentication(c);
 }
 
@@ -1638,7 +1654,7 @@ static inline u8 *ubifs_branch_hash(struct ubifs_info *c,
 static inline void ubifs_copy_hash(const struct ubifs_info *c, const u8 *from,
 				   u8 *to)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_read(c))
 		memcpy(to, from, c->hash_len);
 }
 
@@ -1647,7 +1663,7 @@ int __ubifs_node_insert_hmac(const struct ubifs_info *c, void *buf,
 static inline int ubifs_node_insert_hmac(const struct ubifs_info *c, void *buf,
 					  int len, int ofs_hmac)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		return __ubifs_node_insert_hmac(c, buf, len, ofs_hmac);
 	else
 		return 0;
@@ -1658,7 +1674,7 @@ int __ubifs_node_verify_hmac(const struct ubifs_info *c, const void *buf,
 static inline int ubifs_node_verify_hmac(const struct ubifs_info *c,
 					 const void *buf, int len, int ofs_hmac)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		return __ubifs_node_verify_hmac(c, buf, len, ofs_hmac);
 	else
 		return 0;
@@ -1674,7 +1690,7 @@ static inline int ubifs_node_verify_hmac(const struct ubifs_info *c,
  */
 static inline int ubifs_auth_node_sz(const struct ubifs_info *c)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		return sizeof(struct ubifs_auth_node) + c->hmac_desc_len;
 	else
 		return 0;
@@ -1691,7 +1707,7 @@ static inline int ubifs_shash_copy_state(const struct ubifs_info *c,
 					   struct shash_desc *src,
 					   struct shash_desc *target)
 {
-	if (ubifs_authenticated(c))
+	if (ubifs_authenticated_write(c))
 		return __ubifs_shash_copy_state(c, src, target);
 	else
 		return 0;
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Sascha Hauer]

Hi Torben,

On Thu, Jun 25, 2020 at 05:59:27PM +0200, Torben Hohn wrote:
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 7fc2f3f07c16..ec95f1f50e5e 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
>  			err = -EINVAL;
>  			goto out_free;
>  		}
> +	} else if (c->auth_hash_name) {
> +		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
> +			err = ubifs_init_authentication_read_only(c);
> +			if (err)
> +				goto out_free;
> +		} else {
> +			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
> +				  " authentication support");
> +			err = -EINVAL;
> +			goto out_free;
> +		}
>  	}

In case we don't have a key available for HMAC and can only verify the
FS is correctly signed then we have to be sure that we are mounting
readonly. This means the above needs an additional check for
c->ro_mount.

Once we can be sure that UBIFS is in readonly mode when we can't do HMAC
then there's no point in adding a ubifs_authenticated_write(), because
the places where you call it will never be hit in a readonly mounted
filesystem.

Regards,
 Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Torben Hohn]

On Fri, Jun 26, 2020 at 06:31:20AM +0200, Sascha Hauer wrote:
> Hi Torben,
> 
> On Thu, Jun 25, 2020 at 05:59:27PM +0200, Torben Hohn wrote:
> > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > index 7fc2f3f07c16..ec95f1f50e5e 100644
> > --- a/fs/ubifs/super.c
> > +++ b/fs/ubifs/super.c
> > @@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
> >  			err = -EINVAL;
> >  			goto out_free;
> >  		}
> > +	} else if (c->auth_hash_name) {
> > +		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
> > +			err = ubifs_init_authentication_read_only(c);
> > +			if (err)
> > +				goto out_free;
> > +		} else {
> > +			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
> > +				  " authentication support");
> > +			err = -EINVAL;
> > +			goto out_free;
> > +		}
> >  	}
> 
> In case we don't have a key available for HMAC and can only verify the
> FS is correctly signed then we have to be sure that we are mounting
> readonly. This means the above needs an additional check for
> c->ro_mount.

Indeed, i had that check in authenticate_sb_node() in an earlier
version, and forgot to add it here.

Will do.

> 
> Once we can be sure that UBIFS is in readonly mode when we can't do HMAC
> then there's no point in adding a ubifs_authenticated_write(), because
> the places where you call it will never be hit in a readonly mounted
> filesystem.

The point is making sure, that it really is never hit in a readonly
filesystem. Now, and in the future. If we miss one point, we might
trigger the hmac code with an empty hmac. Although it might just crash.


Maybe we could generate a random key, to lessen the impact of such
an error. But i doubt that i have enough entropy to make that more than
a fig leaf.


Another topic:

Richard raised the point on irc, that in a dirty filesystem, the journal
would be replayed even in ro mode.

i dont see that happening.

--------------------------------------------------------------------------
static int mount_ubifs(struct ubifs_info *c)
{
	[...]

	if (!c->ro_mount) {

	   [...]

	} else if (c->need_recovery) {
		err = ubifs_recover_size(c, false);
		if (err)
			goto out_orphans;
	} else {
		/*
		 * Even if we mount read-only, we have to set space in GC LEB
		 * to proper value because this affects UBIFS free space
		 * reporting. We do not want to have a situation when
		 * re-mounting from R/O to R/W changes amount of free space.
		 */
		err = take_gc_lnum(c);
		if (err)
			goto out_orphans;
	}

	[...]

}
--------------------------------------------------------------------------

ubifs_recover_size() with in_place = false, does not seem to write.



> 
> Regards,
>  Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Another topic:
> 
> Richard raised the point on irc, that in a dirty filesystem, the journal
> would be replayed even in ro mode.
> 
> i dont see that happening.

I think there is a confusion. Replay is not the same as recovery.
Of course it does not recover, this needs writing.
But it has to replay the journal, otherwise the index tree can point to already
garbage collected LEBs.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Sascha Hauer]

On Fri, Jun 26, 2020 at 09:27:14AM +0200, Torben Hohn wrote:
> On Fri, Jun 26, 2020 at 06:31:20AM +0200, Sascha Hauer wrote:
> > Hi Torben,
> > 
> > On Thu, Jun 25, 2020 at 05:59:27PM +0200, Torben Hohn wrote:
> > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > > index 7fc2f3f07c16..ec95f1f50e5e 100644
> > > --- a/fs/ubifs/super.c
> > > +++ b/fs/ubifs/super.c
> > > @@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
> > >  			err = -EINVAL;
> > >  			goto out_free;
> > >  		}
> > > +	} else if (c->auth_hash_name) {
> > > +		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
> > > +			err = ubifs_init_authentication_read_only(c);
> > > +			if (err)
> > > +				goto out_free;
> > > +		} else {
> > > +			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
> > > +				  " authentication support");
> > > +			err = -EINVAL;
> > > +			goto out_free;
> > > +		}
> > >  	}
> > 
> > In case we don't have a key available for HMAC and can only verify the
> > FS is correctly signed then we have to be sure that we are mounting
> > readonly. This means the above needs an additional check for
> > c->ro_mount.
> 
> Indeed, i had that check in authenticate_sb_node() in an earlier
> version, and forgot to add it here.
> 
> Will do.
> 
> > 
> > Once we can be sure that UBIFS is in readonly mode when we can't do HMAC
> > then there's no point in adding a ubifs_authenticated_write(), because
> > the places where you call it will never be hit in a readonly mounted
> > filesystem.
> 
> The point is making sure, that it really is never hit in a readonly
> filesystem. Now, and in the future. If we miss one point, we might
> trigger the hmac code with an empty hmac. Although it might just crash.

If that's your point then you can add a ubifs_assert(c, c->ro_mount) at
those places. This has the advantage that it triggers not only in
authenticated mode, but also in unauthenticated mode. Please add this
assertion explicitly and not indirectly in ubifs_authenticated_write().
This function has a strange semantics, the name suggests that it returns
the status of authenticated write. It's quite unexpected to me that it
triggers a warning when called with only readonly authentication
available.

Regards,
 Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

[Torben Hohn]

On Fri, Jun 26, 2020 at 10:10:28AM +0200, Sascha Hauer wrote:
> On Fri, Jun 26, 2020 at 09:27:14AM +0200, Torben Hohn wrote:
> > On Fri, Jun 26, 2020 at 06:31:20AM +0200, Sascha Hauer wrote:
> > > Hi Torben,
> > > 
> > > On Thu, Jun 25, 2020 at 05:59:27PM +0200, Torben Hohn wrote:
> > > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > > > index 7fc2f3f07c16..ec95f1f50e5e 100644
> > > > --- a/fs/ubifs/super.c
> > > > +++ b/fs/ubifs/super.c
> > > > @@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
> > > >  			err = -EINVAL;
> > > >  			goto out_free;
> > > >  		}
> > > > +	} else if (c->auth_hash_name) {
> > > > +		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
> > > > +			err = ubifs_init_authentication_read_only(c);
> > > > +			if (err)
> > > > +				goto out_free;
> > > > +		} else {
> > > > +			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
> > > > +				  " authentication support");
> > > > +			err = -EINVAL;
> > > > +			goto out_free;
> > > > +		}
> > > >  	}
> > > 
> > > In case we don't have a key available for HMAC and can only verify the
> > > FS is correctly signed then we have to be sure that we are mounting
> > > readonly. This means the above needs an additional check for
> > > c->ro_mount.
> > 
> > Indeed, i had that check in authenticate_sb_node() in an earlier
> > version, and forgot to add it here.
> > 
> > Will do.
> > 
> > > 
> > > Once we can be sure that UBIFS is in readonly mode when we can't do HMAC
> > > then there's no point in adding a ubifs_authenticated_write(), because
> > > the places where you call it will never be hit in a readonly mounted
> > > filesystem.
> > 
> > The point is making sure, that it really is never hit in a readonly
> > filesystem. Now, and in the future. If we miss one point, we might
> > trigger the hmac code with an empty hmac. Although it might just crash.
> 
> If that's your point then you can add a ubifs_assert(c, c->ro_mount) at
> those places. This has the advantage that it triggers not only in
> authenticated mode, but also in unauthenticated mode. Please add this
> assertion explicitly and not indirectly in ubifs_authenticated_write().
> This function has a strange semantics, the name suggests that it returns
> the status of authenticated write. It's quite unexpected to me that it
> triggers a warning when called with only readonly authentication
> available.

Ok. will do.


> 
> Regards,
>  Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Richard Weinberger]

Torben,

----- Ursprüngliche Mail -----
> Von: "Torben Hohn" <torben.hohn@linutronix.de>
> An: "richard" <richard@nod.at>
> CC: "bigeasy" <bigeasy@linutronix.de>, "tglx" <tglx@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha
> Hauer" <s.hauer@pengutronix.de>
> Gesendet: Donnerstag, 25. Juni 2020 17:59:26
> Betreff: [PATCH 0/1] ubifs: support authentication without hmac

> This patch adds support for ubifs authentication without HMAC,
> which obviously only works for a read-only mount.
> 
> ubiblock and dm-verity are not supported by u-boot, and
> the kernel on the target is loaded by u-boot out of the RFS.

As I said on IRC yesterday. There is a bug with UBIFS versioning.
u-boot is not supposed to read from authenticated UBIFS since it has
no authentication support at all an might trip over changed data structures.

We forgot to raise the UBIFS version to w5r1 for authenticated images
and only introduced a new feature flag.
This causes old UBIFS implementations like u-boot's to not enforce the
super block feature flag field. 
Before w4 feature flags didn't get enfocred. :-(

Patches for mkfs.ubifs and kernel are on their way, I just need to carefully
test them with many different old images, u-boot and kernel combinations.

I think it is high noon that u-boot updates their UBIFS, then a
non-authenticated should be trivial.
Of course you need to verify all files you read from it manually then.

> This is a first try to implement this.
> It boots fine, and the WARN_ON is not triggered.
> 
> I plan to update the docs also, but i would like to have
> some positive comments on this before.

I think this is a useful feature, please give me a few day to think about
all implications.

That said, I'm not really a fan of reading files from UBIFS in u-boot.
We have UBI static volumes for that...
Maybe you can give this a try and void some complexity in the bootloader.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Alexander Dahl]

Hello Richard,

Am Freitag, 26. Juni 2020, 10:09:14 CEST schrieb Richard Weinberger:
> That said, I'm not really a fan of reading files from UBIFS in u-boot.
> We have UBI static volumes for that...
> Maybe you can give this a try and void some complexity in the bootloader.

Surprised to hear that. I saw some boards lately doing exactly that. Things 
like kernel image are copied in Linux to e.g. /boot and U-Boot just picks it 
up from there.

I tried to find out more about using UBI static volumes in general and for 
that usecase, but neither http://linux-mtd.infradead.org/doc/ubi.html nor 
http://linux-mtd.infradead.org/faq/ubi.html point me in a direction where to 
find real world examples to look at. Are there any boards in the wild using 
this? Maybe with defconfigs in U-Boot already?

Greets
Alex




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Richard Weinberger]

Alex,

----- Ursprüngliche Mail -----
> Von: "Alexander Dahl" <ada@thorsis.com>
> An: "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha Hauer" <s.hauer@pengutronix.de>
> CC: "richard" <richard@nod.at>, "Torben Hohn" <torben.hohn@linutronix.de>
> Gesendet: Montag, 29. Juni 2020 08:46:10
> Betreff: Re: [PATCH 0/1] ubifs: support authentication without hmac

> Hello Richard,
> 
> Am Freitag, 26. Juni 2020, 10:09:14 CEST schrieb Richard Weinberger:
>> That said, I'm not really a fan of reading files from UBIFS in u-boot.
>> We have UBI static volumes for that...
>> Maybe you can give this a try and void some complexity in the bootloader.
> 
> Surprised to hear that. I saw some boards lately doing exactly that. Things
> like kernel image are copied in Linux to e.g. /boot and U-Boot just picks it
> up from there.

Well, not everyone is using U-Boot. ;-)
 
> I tried to find out more about using UBI static volumes in general and for
> that usecase, but neither http://linux-mtd.infradead.org/doc/ubi.html nor
> http://linux-mtd.infradead.org/faq/ubi.html point me in a direction where to
> find real world examples to look at. Are there any boards in the wild using
> this? Maybe with defconfigs in U-Boot already?

U-Boot choose the UBIFS way AFAICT.

But if you have your own minimal loader you can read from an UBI static volume with
a few lines of C (~200 LoC). This is what I see/use most of the time.
Using static volumes in U-Boot is also possible, IIRC just use "ubi read".

From http://www.linux-mtd.infradead.org/doc/ubi.html#L_overview:
"""
There are 2 types of UBI volumes: dynamic volumes and static volumes. Static volumes are read-only and their contents are protected by CRC-32 checksums, while dynamic volumes are read-write and the upper layers (e.g., a file-system) are responsible for ensuring data integrity.

Static volumes are typically used for the kernel, initramfs, and dtb. Larger static volumes may incur a significant penalty when opening, as the CRC-32 needs to be calculated at this time. If you are looking to use static volumes for anything besides the kernel, initramfs, or dtb you are likely doing something wrong and would be better off using a dynamic volume instead.
"""

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Wolfgang Denk]

Dear Richard,

In message <1067478399.71066.1593414287623.JavaMail.zimbra@nod.at> you wrote:
> 
> > I tried to find out more about using UBI static volumes in general and for
> > that usecase, but neither http://linux-mtd.infradead.org/doc/ubi.html nor
> > http://linux-mtd.infradead.org/faq/ubi.html point me in a direction where to
> > find real world examples to look at. Are there any boards in the wild using
> > this? Maybe with defconfigs in U-Boot already?
> 
> U-Boot choose the UBIFS way AFAICT.

No, U-Boot does not implement any preferences here, i. e. both
methods can be used out of the box.  It is just the preference of
the user who decides which way to go - using the file system seems
more flexible to some unsers, while others care about the additional
boot time needed to mount the file system or the additional memory
footprint needed for the file system code.

Your choice.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Have you lived in this village all your life?"        "No, not yet."

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Richard Weinberger]

Wolfgang,

----- Ursprüngliche Mail -----
> Von: "Wolfgang Denk" <wd@denx.de>
> An: "richard" <richard@nod.at>
> CC: "Alexander Dahl" <ada@thorsis.com>, "Sascha Hauer" <s.hauer@pengutronix.de>, "linux-mtd"
> <linux-mtd@lists.infradead.org>, "Torben Hohn" <torben.hohn@linutronix.de>
> Gesendet: Montag, 29. Juni 2020 09:48:14
> Betreff: Re: [PATCH 0/1] ubifs: support authentication without hmac

> Dear Richard,
> 
> In message <1067478399.71066.1593414287623.JavaMail.zimbra@nod.at> you wrote:
>> 
>> > I tried to find out more about using UBI static volumes in general and for
>> > that usecase, but neither http://linux-mtd.infradead.org/doc/ubi.html nor
>> > http://linux-mtd.infradead.org/faq/ubi.html point me in a direction where to
>> > find real world examples to look at. Are there any boards in the wild using
>> > this? Maybe with defconfigs in U-Boot already?
>> 
>> U-Boot choose the UBIFS way AFAICT.
> 
> No, U-Boot does not implement any preferences here, i. e. both
> methods can be used out of the box.  It is just the preference of
> the user who decides which way to go - using the file system seems
> more flexible to some unsers, while others care about the additional
> boot time needed to mount the file system or the additional memory
> footprint needed for the file system code.
> 
> Your choice.

Good to know! Thanks for pointing this out.

I think for most users reading from a filesystem feels more natural
since on other storage systems this is the only way to go.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Wolfgang Denk]

Dear Richard,

In message <667429184.71186.1593417085045.JavaMail.zimbra@nod.at> you wrote:
>
> > No, U-Boot does not implement any preferences here, i. e. both
> > methods can be used out of the box.  It is just the preference of
> > the user who decides which way to go - using the file system seems
> > more flexible to some unsers, while others care about the additional
> > boot time needed to mount the file system or the additional memory
> > footprint needed for the file system code.
> > 
> > Your choice.
>
> Good to know! Thanks for pointing this out.
>
> I think for most users reading from a filesystem feels more natural
> since on other storage systems this is the only way to go.

It's also easier to use a file system for example during development,
for example when you often switch between several different kernel
images and such.

But as mentioned it comes with the penalty of additional overhead,
both in memory footprint and boot time, so for the production
configuration it makes sense to use a static UBI volume instead.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"The question of whether a computer can think is no more  interesting
than the question of whether a submarine can swim"
                                                - Edsgar W.  Dijkstra

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Richard Weinberger]

Alex,

On Mon, Jun 29, 2020 at 9:11 AM Richard Weinberger <richard@nod.at> wrote:

> U-Boot choose the UBIFS way AFAICT.
>
> But if you have your own minimal loader you can read from an UBI static volume with
> a few lines of C (~200 LoC). This is what I see/use most of the time.
> Using static volumes in U-Boot is also possible, IIRC just use "ubi read".
>
> From http://www.linux-mtd.infradead.org/doc/ubi.html#L_overview:
> """
> There are 2 types of UBI volumes: dynamic volumes and static volumes. Static volumes are read-only and their contents are protected by CRC-32 checksums, while dynamic volumes are read-write and the upper layers (e.g., a file-system) are responsible for ensuring data integrity.
>
> Static volumes are typically used for the kernel, initramfs, and dtb. Larger static volumes may incur a significant penalty when opening, as the CRC-32 needs to be calculated at this time. If you are looking to use static volumes for anything besides the kernel, initramfs, or dtb you are likely doing something wrong and would be better off using a dynamic volume instead.
> """

Did this and Wolfgang's input help?

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/1] ubifs: support authentication without hmac

[Alexander Dahl]

Hello Richard,

thanks for asking back.

Am Dienstag, 30. Juni 2020, 15:36:28 CEST schrieb Richard Weinberger:
> On Mon, Jun 29, 2020 at 9:11 AM Richard Weinberger <richard@nod.at> wrote:
> > U-Boot choose the UBIFS way AFAICT.
> > 
> > But if you have your own minimal loader you can read from an UBI static
> > volume with a few lines of C (~200 LoC). This is what I see/use most of
> > the time. Using static volumes in U-Boot is also possible, IIRC just use
> > "ubi read".
> > 
> > From http://www.linux-mtd.infradead.org/doc/ubi.html#L_overview:
> > """
> > There are 2 types of UBI volumes: dynamic volumes and static volumes.
> > Static volumes are read-only and their contents are protected by CRC-32
> > checksums, while dynamic volumes are read-write and the upper layers
> > (e.g., a file-system) are responsible for ensuring data integrity.
> > 
> > Static volumes are typically used for the kernel, initramfs, and dtb.
> > Larger static volumes may incur a significant penalty when opening, as
> > the CRC-32 needs to be calculated at this time. If you are looking to use
> > static volumes for anything besides the kernel, initramfs, or dtb you are
> > likely doing something wrong and would be better off using a dynamic
> > volume instead. """
> 
> Did this and Wolfgang's input help?

Yes, it did.  I see the benefits of static UBI volumes, and I wish we had 
known this for our projects like 10 years ago.  However I also appreciated 
Wolfgang's input, it's basically what we do with more recent projects.  The 
possibility to easily upgrade kernel image and/or dts speeds up development a 
lot and causes less headaches on system upgrade.

Greets
Alex




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 0/4] ubifs: support authentication without hmac

[Torben Hohn]

This PQ adds support for ubifs authentication without HMAC,
which obviously only works for a read-only mount.

ubiblock and dm-verity are not supported by u-boot, and
the kernel on the target is loaded by u-boot out of the RFS.

This is a first try to implement this.
It boots fine, and the WARN_ON is not triggered.

I plan to update the docs also, but i would like to have
some positive comments on this before.

Changes since v1:

- apply comments from Sascha an revert the
  ubifs_authicated_(read|write) stuff.
  Use ubifs_assert(c, !c->ro_mount) instead.
- Prevent remount rw, when hmac-less authentication is used
- add missing check, for ro mode, when no auth_key_name is specified.

Torben Hohn (4):
  ubifs: move #include "debug.h" above auth.c
  ubifs: support authentication, for ro mount, when no key is given
  ubifs: sprinkle ubifs_assert(c, !c->ro_mount) in hmac auth
  ubifs: prevent remounting rw when no hmac key was given

 fs/ubifs/auth.c    | 62 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/ubifs/gc.c      |  1 +
 fs/ubifs/journal.c |  8 ++++++
 fs/ubifs/replay.c  |  1 +
 fs/ubifs/sb.c      |  5 ++++
 fs/ubifs/super.c   | 28 ++++++++++++++++++++-
 fs/ubifs/ubifs.h   | 12 ++++++++-
 7 files changed, 114 insertions(+), 3 deletions(-)

-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/4] ubifs: move #include "debug.h" above auth.c

[Torben Hohn]

prepare to use ubifs_assert() in some inline functions from
the auth.c block.

move #include "debug.h" above the auth.c block.

Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
---
 fs/ubifs/ubifs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index bff682309fbe..95ed45022e51 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1505,6 +1505,8 @@ extern const struct inode_operations ubifs_dir_inode_operations;
 extern const struct inode_operations ubifs_symlink_inode_operations;
 extern struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
 
+#include "debug.h"
+
 /* auth.c */
 static inline int ubifs_authenticated(const struct ubifs_info *c)
 {
@@ -2063,7 +2065,6 @@ void ubifs_compress(const struct ubifs_info *c, const void *in_buf, int in_len,
 int ubifs_decompress(const struct ubifs_info *c, const void *buf, int len,
 		     void *out, int *out_len, int compr_type);
 
-#include "debug.h"
 #include "misc.h"
 #include "key.h"
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/4] ubifs: support authentication, for ro mount, when no key is given

[Torben Hohn]

Ubifs authentication requires a hmac key, even when a
filesystem is mounted read-only.

Implement ubifs_init_authentication_read_only(),
which only allocates the structures needed for validating
the hashes.

Call ubifs_init_authentication_read_only() when no
auth_key_name is specified, and the filesystem is to be
mounted read only.

Fixup __ubifs_exit_authentication() to free c->hmac_tfm only
when !c->ro_mount.

Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
---
 fs/ubifs/auth.c  | 61 +++++++++++++++++++++++++++++++++++++++++++++++-
 fs/ubifs/sb.c    |  4 ++++
 fs/ubifs/super.c | 19 ++++++++++++++-
 fs/ubifs/ubifs.h |  1 +
 4 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c
index cc5c0abfd536..52ce7a2218a5 100644
--- a/fs/ubifs/auth.c
+++ b/fs/ubifs/auth.c
@@ -248,6 +248,61 @@ int ubifs_sb_verify_signature(struct ubifs_info *c,
 	return err;
 }
 
+/**
+ * ubifs_init_authentication_read_only - init only the read_only parts
+ *
+ * @c: UBIFS file-system description object
+ *
+ * This function returns 0 for success or a negative error code otherwise.
+ */
+
+int ubifs_init_authentication_read_only(struct ubifs_info *c)
+{
+	int err;
+
+	if (!c->auth_hash_name) {
+		ubifs_err(c, "authentication hash name needed with authentication");
+		return -EINVAL;
+	}
+
+	c->auth_hash_algo = match_string(hash_algo_name, HASH_ALGO__LAST,
+					 c->auth_hash_name);
+	if ((int)c->auth_hash_algo < 0) {
+		ubifs_err(c, "Unknown hash algo %s specified",
+			  c->auth_hash_name);
+		return -EINVAL;
+	}
+
+	c->hash_tfm = crypto_alloc_shash(c->auth_hash_name, 0, 0);
+	if (IS_ERR(c->hash_tfm)) {
+		err = PTR_ERR(c->hash_tfm);
+		ubifs_err(c, "Can not allocate %s: %d",
+			  c->auth_hash_name, err);
+		goto out;
+	}
+
+	c->hash_len = crypto_shash_digestsize(c->hash_tfm);
+	if (c->hash_len > UBIFS_HASH_ARR_SZ) {
+		ubifs_err(c, "hash %s is bigger than maximum allowed hash size (%d > %d)",
+			  c->auth_hash_name, c->hash_len, UBIFS_HASH_ARR_SZ);
+		err = -EINVAL;
+		goto out_free_hash;
+	}
+
+	c->authenticated = true;
+
+	c->log_hash = ubifs_hash_get_desc(c);
+	if (IS_ERR(c->log_hash))
+		goto out_free_hash;
+
+	err = 0;
+out_free_hash:
+	if (err)
+		crypto_free_shash(c->hash_tfm);
+out:
+	return err;
+}
+
 /**
  * ubifs_init_authentication - initialize UBIFS authentication support
  * @c: UBIFS file-system description object
@@ -367,9 +422,13 @@ void __ubifs_exit_authentication(struct ubifs_info *c)
 	if (!ubifs_authenticated(c))
 		return;
 
-	crypto_free_shash(c->hmac_tfm);
 	crypto_free_shash(c->hash_tfm);
 	kfree(c->log_hash);
+
+	if (c->ro_mount)
+		return;
+
+	crypto_free_shash(c->hmac_tfm);
 }
 
 /**
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 4b4b65b48c57..d898ea5edd7c 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -583,6 +583,10 @@ static int authenticate_sb_node(struct ubifs_info *c,
 	if (ubifs_hmac_zero(c, sup->hmac)) {
 		err = ubifs_sb_verify_signature(c, sup);
 	} else {
+		if (!c->hmac_tfm) {
+			ubifs_err(c, "HMAC authenticated FS found, but no key given");
+			return -EINVAL;
+		}
 		err = ubifs_hmac_wkm(c, hmac_wkm);
 		if (err)
 			return err;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7fc2f3f07c16..13175da14464 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1291,6 +1291,23 @@ static int mount_ubifs(struct ubifs_info *c)
 			err = -EINVAL;
 			goto out_free;
 		}
+	} else if (c->auth_hash_name) {
+		if (!c->ro_mount) {
+			ubifs_err(c, "auth_hash_name without auth_key_name, but no ro mount");
+			err = -EINVAL;
+			goto out_free;
+		}
+
+		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
+			err = ubifs_init_authentication_read_only(c);
+			if (err)
+				goto out_free;
+		} else {
+			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
+				  " authentication support");
+			err = -EINVAL;
+			goto out_free;
+		}
 	}
 
 	err = ubifs_read_superblock(c);
@@ -1383,7 +1400,7 @@ static int mount_ubifs(struct ubifs_info *c)
 	 * in the superblock, we can update the offline signed
 	 * superblock with a HMAC version,
 	 */
-	if (ubifs_authenticated(c) && ubifs_hmac_zero(c, c->sup_node->hmac)) {
+	if (!c->ro_mount && c->authenticated && ubifs_hmac_zero(c, c->sup_node->hmac)) {
 		err = ubifs_hmac_wkm(c, c->sup_node->hmac_wkm);
 		if (err)
 			goto out_lpt;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 95ed45022e51..80e2800927ec 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1607,6 +1607,7 @@ static inline int ubifs_node_check_hash(const struct ubifs_info *c,
 		return 0;
 }
 
+int ubifs_init_authentication_read_only(struct ubifs_info *c);
 int ubifs_init_authentication(struct ubifs_info *c);
 void __ubifs_exit_authentication(struct ubifs_info *c);
 static inline void ubifs_exit_authentication(struct ubifs_info *c)
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/4] ubifs: sprinkle ubifs_assert(c, !c->ro_mount) in hmac auth

[Torben Hohn]

make sure that the hmac ubifs authentication code, which
uses the hmac is only called, for rw mounts.

This allows to ensure that we are not in the new
read only authentication mode, when the hmac is accessed.

Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
---
 fs/ubifs/auth.c    | 1 +
 fs/ubifs/gc.c      | 1 +
 fs/ubifs/journal.c | 8 ++++++++
 fs/ubifs/replay.c  | 1 +
 fs/ubifs/sb.c      | 1 +
 fs/ubifs/super.c   | 5 +++++
 fs/ubifs/ubifs.h   | 8 ++++++++
 7 files changed, 25 insertions(+)

diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c
index 52ce7a2218a5..15a1a18a42ed 100644
--- a/fs/ubifs/auth.c
+++ b/fs/ubifs/auth.c
@@ -570,6 +570,7 @@ int ubifs_hmac_wkm(struct ubifs_info *c, u8 *hmac)
 	int err;
 	const char well_known_message[] = "UBIFS";
 
+	ubifs_assert(c, !c->ro_mount);
 	if (!ubifs_authenticated(c))
 		return 0;
 
diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
index 62cb3db44e6e..8009ceb362d2 100644
--- a/fs/ubifs/gc.c
+++ b/fs/ubifs/gc.c
@@ -410,6 +410,7 @@ static int move_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb)
 			moved = 1;
 		}
 
+		ubifs_assert(c, !c->ro_mount);
 		if (ubifs_authenticated(c) && moved) {
 			struct ubifs_auth_node *auth;
 
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index e5ec1afe1c66..5c23846f2d40 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -80,6 +80,7 @@ static inline void zero_trun_node_unused(struct ubifs_trun_node *trun)
 
 static void ubifs_add_auth_dirt(struct ubifs_info *c, int lnum)
 {
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c))
 		ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c));
 }
@@ -278,7 +279,9 @@ static int write_head(struct ubifs_info *c, int jhead, void *buf, int len,
 	dbg_jnl("jhead %s, LEB %d:%d, len %d",
 		dbg_jhead(jhead), *lnum, *offs, len);
 
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c)) {
+
 		err = ubifs_hash_nodes(c, buf, len, c->jheads[jhead].log_hash);
 		if (err)
 			return err;
@@ -572,6 +575,7 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 
 	len = aligned_dlen + aligned_ilen + UBIFS_INO_NODE_SZ;
 	/* Make sure to also account for extended attributes */
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c))
 		len += ALIGN(host_ui->data_len, 8) + ubifs_auth_node_sz(c);
 	else
@@ -778,6 +782,8 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
 	}
 
 	dlen = UBIFS_DATA_NODE_SZ + out_len;
+
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c))
 		write_len = ALIGN(dlen, 8) + auth_len;
 	else
@@ -860,6 +866,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode)
 		write_len += UBIFS_INO_NODE_SZ * ui->xattr_cnt;
 	}
 
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c))
 		write_len += ALIGN(ilen, 8) + ubifs_auth_node_sz(c);
 	else
@@ -1572,6 +1579,7 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode,
 	/* Must make reservation before allocating sequence numbers */
 	len = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ;
 
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c))
 		len += ALIGN(dlen, 8) + ubifs_auth_node_sz(c);
 	else
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index b69ffac7e415..a3a37c196f56 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -595,6 +595,7 @@ static int authenticate_sleb(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 	u8 hash[UBIFS_HASH_ARR_SZ];
 	u8 hmac[UBIFS_HMAC_ARR_SZ];
 
+	ubifs_assert(c, !c->ro_mount);
 	if (!ubifs_authenticated(c))
 		return sleb->nodes_cnt;
 
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index d898ea5edd7c..c2104235fb56 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -176,6 +176,7 @@ static int create_default_filesystem(struct ubifs_info *c)
 		sup_flags |= UBIFS_FLG_BIGLPT;
 	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
 
+	ubifs_assert(c, !c->ro_mount);
 	if (ubifs_authenticated(c)) {
 		sup_flags |= UBIFS_FLG_AUTHENTICATION;
 		sup->hash_algo = cpu_to_le16(c->auth_hash_algo);
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 13175da14464..b41ca9ee5763 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1447,6 +1447,7 @@ static int mount_ubifs(struct ubifs_info *c)
 		}
 
 		if (c->need_recovery) {
+			ubifs_assert(c, !c->ro_mount);
 			if (!ubifs_authenticated(c)) {
 				err = ubifs_recover_size(c, true);
 				if (err)
@@ -1457,6 +1458,7 @@ static int mount_ubifs(struct ubifs_info *c)
 			if (err)
 				goto out_orphans;
 
+			ubifs_assert(c, !c->ro_mount);
 			if (ubifs_authenticated(c)) {
 				err = ubifs_recover_size(c, false);
 				if (err)
@@ -1703,6 +1705,8 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 		err = ubifs_write_rcvrd_mst_node(c);
 		if (err)
 			goto out;
+
+		ubifs_assert(c, !c->ro_mount);
 		if (!ubifs_authenticated(c)) {
 			err = ubifs_recover_size(c, true);
 			if (err)
@@ -1788,6 +1792,7 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 		if (err)
 			goto out;
 
+		ubifs_assert(c, !c->ro_mount);
 		if (ubifs_authenticated(c)) {
 			err = ubifs_recover_size(c, false);
 			if (err)
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 80e2800927ec..79880cf0e1a0 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1650,6 +1650,8 @@ int __ubifs_node_insert_hmac(const struct ubifs_info *c, void *buf,
 static inline int ubifs_node_insert_hmac(const struct ubifs_info *c, void *buf,
 					  int len, int ofs_hmac)
 {
+	ubifs_assert(c, !c->ro_mount);
+
 	if (ubifs_authenticated(c))
 		return __ubifs_node_insert_hmac(c, buf, len, ofs_hmac);
 	else
@@ -1661,6 +1663,8 @@ int __ubifs_node_verify_hmac(const struct ubifs_info *c, const void *buf,
 static inline int ubifs_node_verify_hmac(const struct ubifs_info *c,
 					 const void *buf, int len, int ofs_hmac)
 {
+	ubifs_assert(c, !c->ro_mount);
+
 	if (ubifs_authenticated(c))
 		return __ubifs_node_verify_hmac(c, buf, len, ofs_hmac);
 	else
@@ -1677,6 +1681,8 @@ static inline int ubifs_node_verify_hmac(const struct ubifs_info *c,
  */
 static inline int ubifs_auth_node_sz(const struct ubifs_info *c)
 {
+	ubifs_assert(c, !c->ro_mount);
+
 	if (ubifs_authenticated(c))
 		return sizeof(struct ubifs_auth_node) + c->hmac_desc_len;
 	else
@@ -1694,6 +1700,8 @@ static inline int ubifs_shash_copy_state(const struct ubifs_info *c,
 					   struct shash_desc *src,
 					   struct shash_desc *target)
 {
+	ubifs_assert(c, !c->ro_mount);
+
 	if (ubifs_authenticated(c))
 		return __ubifs_shash_copy_state(c, src, target);
 	else
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given

[Torben Hohn]

After adding readonly hmac-less authentication support,
prevent remounting the filesystem in rw mode, when
the hmac is not available.

Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
---
 fs/ubifs/super.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index b41ca9ee5763..62bdef8f1ddf 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1996,6 +1996,10 @@ static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
 			ubifs_msg(c, "cannot re-mount R/W - UBI volume is R/O");
 			return -EROFS;
 		}
+		if (ubifs_authenticated(c) && !c->hash_tfm) {
+			ubifs_msg(c, "cannot re-mount R/W due to missing hmac key, for authentication");
+			return -EROFS;
+		}
 		err = ubifs_remount_rw(c);
 		if (err)
 			return err;
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Torben Hohn" <torben.hohn@linutronix.de>
> An: "richard" <richard@nod.at>
> CC: "bigeasy" <bigeasy@linutronix.de>, "tglx" <tglx@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha
> Hauer" <s.hauer@pengutronix.de>
> Gesendet: Freitag, 26. Juni 2020 13:29:07
> Betreff: [PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given

> After adding readonly hmac-less authentication support,
> prevent remounting the filesystem in rw mode, when
> the hmac is not available.
> 
> Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
> ---
> fs/ubifs/super.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index b41ca9ee5763..62bdef8f1ddf 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1996,6 +1996,10 @@ static int ubifs_remount_fs(struct super_block *sb, int
> *flags, char *data)
> 			ubifs_msg(c, "cannot re-mount R/W - UBI volume is R/O");
> 			return -EROFS;
> 		}
> +		if (ubifs_authenticated(c) && !c->hash_tfm) {
> +			ubifs_msg(c, "cannot re-mount R/W due to missing hmac key, for
> authentication");
> +			return -EROFS;
> +		}

But the case that one remounts rw and provides a HAMC is handled?

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given

[Torben Hohn]

On Fri, Jun 26, 2020 at 02:27:21PM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Torben Hohn" <torben.hohn@linutronix.de>
> > An: "richard" <richard@nod.at>
> > CC: "bigeasy" <bigeasy@linutronix.de>, "tglx" <tglx@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha
> > Hauer" <s.hauer@pengutronix.de>
> > Gesendet: Freitag, 26. Juni 2020 13:29:07
> > Betreff: [PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given
> 
> > After adding readonly hmac-less authentication support,
> > prevent remounting the filesystem in rw mode, when
> > the hmac is not available.
> > 
> > Signed-off-by: Torben Hohn <torben.hohn@linutronix.de>
> > ---
> > fs/ubifs/super.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > index b41ca9ee5763..62bdef8f1ddf 100644
> > --- a/fs/ubifs/super.c
> > +++ b/fs/ubifs/super.c
> > @@ -1996,6 +1996,10 @@ static int ubifs_remount_fs(struct super_block *sb, int
> > *flags, char *data)
> > 			ubifs_msg(c, "cannot re-mount R/W - UBI volume is R/O");
> > 			return -EROFS;
> > 		}
> > +		if (ubifs_authenticated(c) && !c->hash_tfm) {
> > +			ubifs_msg(c, "cannot re-mount R/W due to missing hmac key, for
> > authentication");
> > +			return -EROFS;
> > +		}
> 
> But the case that one remounts rw and provides a HAMC is handled?

No. And i am not sure, whether thats a thing, we want to support.
I would suggest, we clarify that in the message.


One would need to check, whether c->auth_key_name is set now,
and then instantiate c->hash_tfm.

ubifs_init_authentication is not called upon remount.
so a remount with a changed auth_key_name is not supported either.




> 
> Thanks,
> //richard

-- 
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 4/4] ubifs: prevent remounting rw when no hmac key was given

[Richard Weinberger]

Torben,

----- Ursprüngliche Mail -----
>> 
>> But the case that one remounts rw and provides a HAMC is handled?
> 
> No. And i am not sure, whether thats a thing, we want to support.
> I would suggest, we clarify that in the message.

Yeah.
 
> 
> One would need to check, whether c->auth_key_name is set now,
> and then instantiate c->hash_tfm.
> 
> ubifs_init_authentication is not called upon remount.
> so a remount with a changed auth_key_name is not supported either.

Because it is technically not possible. We'd need both keys then.
But I agree we should reject it. Remounting is tricky...

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

Torben,

----- Ursprüngliche Mail -----
> Von: "Torben Hohn" <torben.hohn@linutronix.de>
> An: "richard" <richard@nod.at>
> CC: "bigeasy" <bigeasy@linutronix.de>, "tglx" <tglx@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha
> Hauer" <s.hauer@pengutronix.de>
> Gesendet: Freitag, 26. Juni 2020 13:29:03
> Betreff: [PATCH v2 0/4] ubifs: support authentication without hmac

> This PQ adds support for ubifs authentication without HMAC,
> which obviously only works for a read-only mount.
> 
> ubiblock and dm-verity are not supported by u-boot, and
> the kernel on the target is loaded by u-boot out of the RFS.
> 
> This is a first try to implement this.
> It boots fine, and the WARN_ON is not triggered.
> 
> I plan to update the docs also, but i would like to have
> some positive comments on this before.
> 
> Changes since v1:
> 
> - apply comments from Sascha an revert the
>  ubifs_authicated_(read|write) stuff.
>  Use ubifs_assert(c, !c->ro_mount) instead.
> - Prevent remount rw, when hmac-less authentication is used
> - add missing check, for ro mode, when no auth_key_name is specified.

I didn't dig deep into the code so far, I'm still checking the concept.

Your approach works only on pristine offline signed images from mkfs.ubifs.
So, if somebody does this, it won't work:

$ keyctl padd logon ubifs:authfs @s < secret.key 
$ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,auth_key=ubifs:authfs

... change the fs ...

$ umount /mnt
$ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,ro

The ro mount will fail because UBIFS is no longer able to verify the super block
using the system key ring. It was overwritten by they ubifs:authfs key.

A possible solution is keeping a copy of the offline sign key forever in the fs.
But I'm not sure whether this is wise.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

----- Ursprüngliche Mail -----
> I didn't dig deep into the code so far, I'm still checking the concept.
> 
> Your approach works only on pristine offline signed images from mkfs.ubifs.
> So, if somebody does this, it won't work:
> 
> $ keyctl padd logon ubifs:authfs @s < secret.key
> $ mount -t ubifs /dev/ubi0_0 /mnt/ -o
> auth_hash_name=sha256,auth_key=ubifs:authfs
> 
> ... change the fs ...
> 
> $ umount /mnt
> $ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,ro
> 
> The ro mount will fail because UBIFS is no longer able to verify the super block
> using the system key ring. It was overwritten by they ubifs:authfs key.
> 
> A possible solution is keeping a copy of the offline sign key forever in the fs.
> But I'm not sure whether this is wise.

Or we change the feature from "ro mount without hmac" to "keep offline sign key and imply ro mount".
IOW adding a new mount option such as "auth_keep_offlinekey". If mounted with this option
UBIFS will not look for a hmac and enforce read-only mode.

Hmm?

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Torben Hohn]

On Fri, Jun 26, 2020 at 04:36:26PM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > I didn't dig deep into the code so far, I'm still checking the concept.
> > 
> > Your approach works only on pristine offline signed images from mkfs.ubifs.
> > So, if somebody does this, it won't work:
> > 
> > $ keyctl padd logon ubifs:authfs @s < secret.key
> > $ mount -t ubifs /dev/ubi0_0 /mnt/ -o
> > auth_hash_name=sha256,auth_key=ubifs:authfs
> > 
> > ... change the fs ...
> > 
> > $ umount /mnt
> > $ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,ro
> > 
> > The ro mount will fail because UBIFS is no longer able to verify the super block
> > using the system key ring. It was overwritten by they ubifs:authfs key.
> > 
> > A possible solution is keeping a copy of the offline sign key forever in the fs.
> > But I'm not sure whether this is wise.
> 
> Or we change the feature from "ro mount without hmac" to "keep offline sign key and imply ro mount".
> IOW adding a new mount option such as "auth_keep_offlinekey". If mounted with this option
> UBIFS will not look for a hmac and enforce read-only mode.

Thats just another name for the same feature.
But it indeed seems to make the implications clearer.

And it porbably also makes the code easier to read.

> 
> Hmm?
> 
> Thanks,
> //richard

-- 
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Torben Hohn]

On Fri, Jun 26, 2020 at 04:16:51PM +0200, Richard Weinberger wrote:
> Torben,
> 
> ----- Ursprüngliche Mail -----
> > Von: "Torben Hohn" <torben.hohn@linutronix.de>
> > An: "richard" <richard@nod.at>
> > CC: "bigeasy" <bigeasy@linutronix.de>, "tglx" <tglx@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha
> > Hauer" <s.hauer@pengutronix.de>
> > Gesendet: Freitag, 26. Juni 2020 13:29:03
> > Betreff: [PATCH v2 0/4] ubifs: support authentication without hmac
> 
> > This PQ adds support for ubifs authentication without HMAC,
> > which obviously only works for a read-only mount.
> > 
> > ubiblock and dm-verity are not supported by u-boot, and
> > the kernel on the target is loaded by u-boot out of the RFS.
> > 
> > This is a first try to implement this.
> > It boots fine, and the WARN_ON is not triggered.
> > 
> > I plan to update the docs also, but i would like to have
> > some positive comments on this before.
> > 
> > Changes since v1:
> > 
> > - apply comments from Sascha an revert the
> >  ubifs_authicated_(read|write) stuff.
> >  Use ubifs_assert(c, !c->ro_mount) instead.
> > - Prevent remount rw, when hmac-less authentication is used
> > - add missing check, for ro mode, when no auth_key_name is specified.
> 
> I didn't dig deep into the code so far, I'm still checking the concept.
> 
> Your approach works only on pristine offline signed images from mkfs.ubifs.
> So, if somebody does this, it won't work:
> 
> $ keyctl padd logon ubifs:authfs @s < secret.key 
> $ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,auth_key=ubifs:authfs
> 
> ... change the fs ...
> 
> $ umount /mnt
> $ mount -t ubifs /dev/ubi0_0 /mnt/ -o auth_hash_name=sha256,ro
> 
> The ro mount will fail because UBIFS is no longer able to verify the super block
> using the system key ring. It was overwritten by they ubifs:authfs key.

Yes. But that is the intended behaviour.
If the filesystem has been changed, it must not be mounted again.

I would rather like to make it impossible to mount the filesystem in rw
mode, because this is an attack scenario. It would refuse to mount upon
reboot. Making it possible to remount root rw, with a fresh key is
nice for development, but its not desired in production. 


> 
> A possible solution is keeping a copy of the offline sign key forever in the fs.
> But I'm not sure whether this is wise.

Heh ? you mean the private key. NO

When its not possible to store a secret on the h/w then the only
option we have is asymmetric sigatures, with the private key never
being on the device.

This obviously means, that the device is not able to write to RFS.




> 
> Thanks,
> //richard

-- 
Torben Hohn
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 18; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

Torben,

----- Ursprüngliche Mail -----
>> The ro mount will fail because UBIFS is no longer able to verify the super block
>> using the system key ring. It was overwritten by they ubifs:authfs key.
> 
> Yes. But that is the intended behaviour.
> If the filesystem has been changed, it must not be mounted again.

In your use case.
 
> I would rather like to make it impossible to mount the filesystem in rw
> mode, because this is an attack scenario. It would refuse to mount upon
> reboot. Making it possible to remount root rw, with a fresh key is
> nice for development, but its not desired in production.
> 
> 
>> 
>> A possible solution is keeping a copy of the offline sign key forever in the fs.
>> But I'm not sure whether this is wise.
> 
> Heh ? you mean the private key. NO

No, I used bad wording. :-)
The superblock is signed by the offline key. As soon you switch to the new key
the super block is rewritten and can no longer verified this key.
Instead of rewriting the idea was keeping a copy.

Anyway, like said in the other mail, I think if we change the feature to
"keep offline sign key and imply ro mount" things will be more smooth with less corner
cases.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Thomas Gleixner]

Richard Weinberger <richard@nod.at> writes:
> The superblock is signed by the offline key. As soon you switch to the new key
> the super block is rewritten and can no longer verified this key.
> Instead of rewriting the idea was keeping a copy.
>
> Anyway, like said in the other mail, I think if we change the feature to
> "keep offline sign key and imply ro mount" things will be more smooth with less corner
> cases.

I don't think so. The desired mode is to prevent RW mounts for a factory
signed image which implies the prevention of rewriting the superblock.

This is not a conveniance feature, it's a strict security feature. Once
the thing has been changed from the factory generated state it's invalid
no matter what.

If a developer shoots himself in the foot with that, no big deal. He got
what he asked for :)

Thanks,

        tglx

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "tglx" <tglx@linutronix.de>
> An: "richard" <richard@nod.at>, "Torben Hohn" <torben.hohn@linutronix.de>
> CC: "bigeasy" <bigeasy@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha Hauer"
> <s.hauer@pengutronix.de>, "david" <david@sigma-star.at>
> Gesendet: Donnerstag, 2. Juli 2020 16:40:24
> Betreff: Re: [PATCH v2 0/4] ubifs: support authentication without hmac

> Richard Weinberger <richard@nod.at> writes:
>> The superblock is signed by the offline key. As soon you switch to the new key
>> the super block is rewritten and can no longer verified this key.
>> Instead of rewriting the idea was keeping a copy.
>>
>> Anyway, like said in the other mail, I think if we change the feature to
>> "keep offline sign key and imply ro mount" things will be more smooth with less
>> corner
>> cases.
> 
> I don't think so. The desired mode is to prevent RW mounts for a factory
> signed image which implies the prevention of rewriting the superblock.

This is exactly what I'm asking for.
Keep the factory signed super block and imply read-only mode.
  
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Thomas Gleixner]

Richard Weinberger <richard@nod.at> writes:
> ----- Ursprüngliche Mail -----
>> Von: "tglx" <tglx@linutronix.de>
>> An: "richard" <richard@nod.at>, "Torben Hohn" <torben.hohn@linutronix.de>
>> CC: "bigeasy" <bigeasy@linutronix.de>, "linux-mtd" <linux-mtd@lists.infradead.org>, "Sascha Hauer"
>> <s.hauer@pengutronix.de>, "david" <david@sigma-star.at>
>> Gesendet: Donnerstag, 2. Juli 2020 16:40:24
>> Betreff: Re: [PATCH v2 0/4] ubifs: support authentication without hmac
>
>> Richard Weinberger <richard@nod.at> writes:
>>> The superblock is signed by the offline key. As soon you switch to the new key
>>> the super block is rewritten and can no longer verified this key.
>>> Instead of rewriting the idea was keeping a copy.
>>>
>>> Anyway, like said in the other mail, I think if we change the feature to
>>> "keep offline sign key and imply ro mount" things will be more smooth with less
>>> corner
>>> cases.
>> 
>> I don't think so. The desired mode is to prevent RW mounts for a factory
>> signed image which implies the prevention of rewriting the superblock.
>
> This is exactly what I'm asking for.
> Keep the factory signed super block and imply read-only mode.

And that's what Torben implemented unless I'm missing something.

Thanks,

        tglx

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

----- Ursprüngliche Mail -----
>>>> Anyway, like said in the other mail, I think if we change the feature to
>>>> "keep offline sign key and imply ro mount" things will be more smooth with less
>>>> corner
>>>> cases.
>>> 
>>> I don't think so. The desired mode is to prevent RW mounts for a factory
>>> signed image which implies the prevention of rewriting the superblock.
>>
>> This is exactly what I'm asking for.
>> Keep the factory signed super block and imply read-only mode.
> 
> And that's what Torben implemented unless I'm missing something.

Torben implemented it the other way around, he allows mounting without
the HMAC if UBIFS mount is read-only.
This covers also the proposed use-case but as I stated it has issues with
remounting and makes the implementation more complicated than it should be.

That's why I proposed adding a new mount option like "keep_offline_signature" or
what name fits better. That gives us the following pros:

1. Makes the implementation super simple.
   If keep_offline_signature is set and rw mount requested, reject.
   RW remount can rejected very easily, store keep_offline_signature in the ubifs context.

2. If the super block got already re-written, reject.
   You can check sub->hmac[] for being non-zero.
   That way we can give the user a decent error message in case they do stupid things.

3. Userspace can verify whether the UBIFS fs is pristine by checking
   for the keep_offline_signature mount flag in /proc/self/mountinfo.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[bigeasy]

On 2020-07-02 21:03:54 [+0200], Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> >>>> Anyway, like said in the other mail, I think if we change the feature to
> >>>> "keep offline sign key and imply ro mount" things will be more smooth with less
> >>>> corner
> >>>> cases.
> >>> 
> >>> I don't think so. The desired mode is to prevent RW mounts for a factory
> >>> signed image which implies the prevention of rewriting the superblock.
> >>
> >> This is exactly what I'm asking for.
> >> Keep the factory signed super block and imply read-only mode.
> > 
> > And that's what Torben implemented unless I'm missing something.
> 
> Torben implemented it the other way around, he allows mounting without
> the HMAC if UBIFS mount is read-only.
> This covers also the proposed use-case but as I stated it has issues with
> remounting and makes the implementation more complicated than it should be.
> 
> That's why I proposed adding a new mount option like "keep_offline_signature" or
> what name fits better. That gives us the following pros:

so you want an extra option instead of setting SB_RDONLY on RO mounts
without the key and not allowing RW mounts in this case?

> 1. Makes the implementation super simple.
>    If keep_offline_signature is set and rw mount requested, reject.
>    RW remount can rejected very easily, store keep_offline_signature in the ubifs context.
> 
> 2. If the super block got already re-written, reject.
>    You can check sub->hmac[] for being non-zero.
>    That way we can give the user a decent error message in case they do stupid things.

re-written as in a prior RW mount with the key?

> 3. Userspace can verify whether the UBIFS fs is pristine by checking
>    for the keep_offline_signature mount flag in /proc/self/mountinfo.

Could this information be dubious if the UBIFS was mounted RW once (with
the key around) and then mounted RO,keep_offline_signature ? So you
would have to only allow keep_offline_signature if your point (2) is
true?

> Thanks,
> //richard

Sebastian

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Richard Weinberger]

Sebastian,

----- Ursprüngliche Mail -----
>> > And that's what Torben implemented unless I'm missing something.
>> 
>> Torben implemented it the other way around, he allows mounting without
>> the HMAC if UBIFS mount is read-only.
>> This covers also the proposed use-case but as I stated it has issues with
>> remounting and makes the implementation more complicated than it should be.
>> 
>> That's why I proposed adding a new mount option like "keep_offline_signature" or
>> what name fits better. That gives us the following pros:
> 
> so you want an extra option instead of setting SB_RDONLY on RO mounts
> without the key and not allowing RW mounts in this case?

Yes.

>> 1. Makes the implementation super simple.
>>    If keep_offline_signature is set and rw mount requested, reject.
>>    RW remount can rejected very easily, store keep_offline_signature in the ubifs
>>    context.
>> 
>> 2. If the super block got already re-written, reject.
>>    You can check sub->hmac[] for being non-zero.
>>    That way we can give the user a decent error message in case they do stupid
>>    things.
> 
> re-written as in a prior RW mount with the key?

Yes.

>> 3. Userspace can verify whether the UBIFS fs is pristine by checking
>>    for the keep_offline_signature mount flag in /proc/self/mountinfo.
> 
> Could this information be dubious if the UBIFS was mounted RW once (with
> the key around) and then mounted RO,keep_offline_signature ? So you
> would have to only allow keep_offline_signature if your point (2) is
> true?

No. Because as soon you mount once RW the super block is re-written with
the provided HMAC. You can detect this and refuse the mount option.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/4] ubifs: support authentication without hmac

[Thomas Gleixner]

Richard Weinberger <richard@nod.at> writes:
>> And that's what Torben implemented unless I'm missing something.
>
> Torben implemented it the other way around, he allows mounting without
> the HMAC if UBIFS mount is read-only.
> This covers also the proposed use-case but as I stated it has issues with
> remounting and makes the implementation more complicated than it should be.
>
> That's why I proposed adding a new mount option like "keep_offline_signature" or
> what name fits better. That gives us the following pros:
>
> 1. Makes the implementation super simple.
>    If keep_offline_signature is set and rw mount requested, reject.
>    RW remount can rejected very easily, store keep_offline_signature in the ubifs context.
>
> 2. If the super block got already re-written, reject.
>    You can check sub->hmac[] for being non-zero.
>    That way we can give the user a decent error message in case they do stupid things.
>
> 3. Userspace can verify whether the UBIFS fs is pristine by checking
>    for the keep_offline_signature mount flag in /proc/self/mountinfo.

Works for me.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/