lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1337427288.2041.24.camel@koala>
Date:	Sat, 19 May 2012 14:34:48 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Joel Reardon <joel@...mbassador.com>
Cc:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] UBIFS: read crypto_lookup from datanodes to znodes

On Sat, 2012-05-19 at 10:30 +0200, Joel Reardon wrote:
> Crypto_lookup values are now read from the data node header whenever a
> data node is read from flash and cached in the TNC. The value will be zero
> until it is read from flash. (If it is needed before it happens to be
> loaded, i.e., to delete a node, then it will be forced read.)
> 
> It was tested by setting cryptolookup values for new datanodes by their
> lnum and offset on flash. This was later asserted in the TNC that they
> were either zero, or always equal. On mounting, the TNC crypto lookup
> values were confirmed to be zero and later, after reading the
> corresponding files, confirmed they were loaded and corresponded to the
> location on flash.
> 
> Signed-off-by: Joel Reardon <reardonj@....ethz.ch>

I've pushed this patch with minor amendments to the "joel" branch, and
I've applied the renaming patch on top (it also change some logic a bit
to make it more compact) - if you do not like that patch - complain,
I'll remove it. Also I have a request below. Thanks!

> diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
> index 9ecbd37..e03adcf 100644
> --- a/fs/ubifs/ubifs-media.h
> +++ b/fs/ubifs/ubifs-media.h
> @@ -539,6 +539,7 @@ struct ubifs_dent_node {
>   * struct ubifs_data_node - data node.
>   * @ch: common header
>   * @key: node key
> + * @crypto_lookup: the node's cryptographic key's position in the KSA
>   * @size: uncompressed data size in bytes
>   * @compr_type: compression type (%UBIFS_COMPR_NONE, %UBIFS_COMPR_LZO, etc)
>   * @padding: reserved for future, zeroes
> @@ -550,7 +551,7 @@ struct ubifs_dent_node {
>  struct ubifs_data_node {
>  	struct ubifs_ch ch;
>  	__u8 key[UBIFS_KEY_LEN];
> -	__le64 padding0; /* Watch 'zero_data_node_unused()' if changing! */
> +	__le64 crypto_lookup;
>  	__le32 size;
>  	__le16 compr_type;
>  	__u8 padding1[2]; /* Watch 'zero_data_node_unused()' if changing! */

This file is copied to user-space (mtd-utils.git) and I try to keep it
well-documented. Could you please extend 'struct ubifs_data_node' coment
and explain that older versions of UBIFS did not have secure deletion
support and the 'crypto_lookup' field contained all zeroes.

The patch I've applied on top:

From 87c912b858520939da9ef9e258b01a6fe85a342b Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Date: Sat, 19 May 2012 14:23:01 +0300
Subject: [PATCH] UBIFS: rename crypto_lookup

Rename 'crypto_lookup' to 'ksa_pos' in order to has shorter lines - the old
name is too long. Also makes code around assertions in the node reading
function a bit more compact.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
---
 fs/ubifs/gc.c          |    6 +++---
 fs/ubifs/journal.c     |    4 ++--
 fs/ubifs/tnc.c         |   33 ++++++++++++++-------------------
 fs/ubifs/tnc_misc.c    |   10 ++++------
 fs/ubifs/ubifs-media.h |    4 ++--
 fs/ubifs/ubifs.h       |    8 ++++----
 6 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
index 4a21358..6c84af3 100644
--- a/fs/ubifs/gc.c
+++ b/fs/ubifs/gc.c
@@ -322,7 +322,7 @@ static int move_node(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 		     struct ubifs_scan_node *snod, struct ubifs_wbuf *wbuf)
 {
 	int err, new_lnum = wbuf->lnum, new_offs = wbuf->offs + wbuf->used;
-	long long crypto_lookup = 0;
+	long long ksa_pos = 0;
 
 	cond_resched();
 	err = ubifs_wbuf_write_nolock(wbuf, snod->node, snod->len);
@@ -332,11 +332,11 @@ static int move_node(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 	if (key_type(c, &snod->key) == UBIFS_DATA_KEY) {
 		struct ubifs_data_node *dn = snod->node;
 
-		crypto_lookup = le64_to_cpu(dn->crypto_lookup);
+		ksa_pos = le64_to_cpu(dn->ksa_pos);
 	}
 	err = ubifs_tnc_replace(c, &snod->key, sleb->lnum,
 				snod->offs, new_lnum, new_offs,
-				snod->len, crypto_lookup);
+				snod->len, ksa_pos);
 	list_del(&snod->list);
 	kfree(snod);
 	return err;
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index bd69a30..d1c21d1 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -734,7 +734,7 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
 
 	dlen = UBIFS_DATA_NODE_SZ + out_len;
 	data->compr_type = cpu_to_le16(compr_type);
-	data->crypto_lookup = cpu_to_le64(0);
+	data->ksa_pos = cpu_to_le64(0);
 
 	/* Make reservation before allocating sequence numbers */
 	err = make_reservation(c, DATAHD, dlen);
@@ -1227,7 +1227,7 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode,
 	if (dlen) {
 		sz = offs + UBIFS_INO_NODE_SZ + UBIFS_TRUN_NODE_SZ;
 		err = ubifs_tnc_add(c, &key, lnum, sz, dlen,
-				    le64_to_cpu(dn->crypto_lookup));
+				    le64_to_cpu(dn->ksa_pos));
 		if (err)
 			goto out_ro;
 	}
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index b36d01c..d9de581 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -499,8 +499,8 @@ static int try_read_node(const struct ubifs_info *c, void *buf, int type,
  *
  * This function tries to read a node and returns %1 if the node is read, %0
  * if the node is not present, and a negative error code in the case of error.
- * It sets @zbr's @crypto_lookup field to the value in the read node if it is
- * a data node.
+ * It sets @zbr's @ksa_pos field to the value in the read node if it is a data
+ * node.
  */
 static int fallible_read_node(struct ubifs_info *c, const union ubifs_key *key,
 			      struct ubifs_zbranch *zbr, void *node)
@@ -520,17 +520,12 @@ static int fallible_read_node(struct ubifs_info *c, const union ubifs_key *key,
 		if (keys_cmp(c, key, &node_key) != 0)
 			ret = 0;
 
-		/* If it is actually a data node, read the @crypto_lookup */
+		/* If it is actually a data node, read the @ksa_pos */
 		if (key_type(c, key) == UBIFS_DATA_KEY) {
-			long long crypto_lookup;
+			long long ksa_pos = le64_to_cpu(dn->ksa_pos);
 
-			crypto_lookup = le64_to_cpu(dn->crypto_lookup);
-			if (zbr->crypto_lookup != 0)
-				ubifs_assert(zbr->crypto_lookup ==
-					     crypto_lookup);
-			else
-				zbr->crypto_lookup =
-					crypto_lookup;
+			ubifs_assert(!zbr->ksa_pos || zbr->ksa_pos == ksa_pos);
+			zbr->ksa_pos = ksa_pos;
 		}
 
 	}
@@ -2176,14 +2171,14 @@ do_split:
  * @lnum: LEB number of node
  * @offs: node offset
  * @len: node length
- * @crypto_lookup: the node's cryptographic key's position in the KSA
+ * @ksa_pos: the node's cryptographic key's position in the KSA
  *
  * This function adds a node with key @key to TNC. The node may be new or it may
  * obsolete some existing one. Returns %0 on success or negative error code on
  * failure.
  */
 int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
-		  int offs, int len, long long crypto_lookup)
+		  int offs, int len, long long ksa_pos)
 {
 	int found, n, err = 0;
 	struct ubifs_znode *znode;
@@ -2198,7 +2193,7 @@ int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
 		zbr.lnum = lnum;
 		zbr.offs = offs;
 		zbr.len = len;
-		zbr.crypto_lookup = crypto_lookup;
+		zbr.ksa_pos = ksa_pos;
 		key_copy(c, key, &zbr.key);
 		err = tnc_insert(c, znode, &zbr, n + 1);
 	} else if (found == 1) {
@@ -2209,7 +2204,7 @@ int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
 		zbr->lnum = lnum;
 		zbr->offs = offs;
 		zbr->len = len;
-		zbr->crypto_lookup = crypto_lookup;
+		zbr->ksa_pos = ksa_pos;
 	} else
 		err = found;
 	if (!err)
@@ -2228,7 +2223,7 @@ int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
  * @lnum: LEB number of node
  * @offs: node offset
  * @len: node length
- * @crypto_lookup: the node's updated cryptographic key's position in the KSA
+ * @ksa_pos: the node's updated cryptographic key's position in the KSA
  *
  * This function replaces a node with key @key in the TNC only if the old node
  * is found.  This function is called by garbage collection when node are moved.
@@ -2236,7 +2231,7 @@ int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
  */
 int ubifs_tnc_replace(struct ubifs_info *c, const union ubifs_key *key,
 		      int old_lnum, int old_offs, int lnum, int offs, int len,
-		      long long crypto_lookup)
+		      long long ksa_pos)
 {
 	int found, n, err = 0;
 	struct ubifs_znode *znode;
@@ -2262,7 +2257,7 @@ int ubifs_tnc_replace(struct ubifs_info *c, const union ubifs_key *key,
 			zbr->lnum = lnum;
 			zbr->offs = offs;
 			zbr->len = len;
-			zbr->crypto_lookup = crypto_lookup;
+			zbr->ksa_pos = ksa_pos;
 			found = 1;
 		} else if (is_hash_key(c, key)) {
 			found = resolve_collision_directly(c, key, &znode, &n,
@@ -2500,7 +2495,7 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 			c->zroot.lnum = zbr->lnum;
 			c->zroot.offs = zbr->offs;
 			c->zroot.len = zbr->len;
-			c->zroot.crypto_lookup = zbr->crypto_lookup;
+			c->zroot.ksa_pos = zbr->ksa_pos;
 			c->zroot.znode = znode;
 			ubifs_assert(!ubifs_zn_obsolete(zp));
 			ubifs_assert(ubifs_zn_dirty(zp));
diff --git a/fs/ubifs/tnc_misc.c b/fs/ubifs/tnc_misc.c
index aa8a51d..169c9f3 100644
--- a/fs/ubifs/tnc_misc.c
+++ b/fs/ubifs/tnc_misc.c
@@ -309,7 +309,7 @@ static int read_znode(struct ubifs_info *c, int lnum, int offs, int len,
 		zbr->lnum = le32_to_cpu(br->lnum);
 		zbr->offs = le32_to_cpu(br->offs);
 		zbr->len  = le32_to_cpu(br->len);
-		zbr->crypto_lookup = 0;
+		zbr->ksa_pos = 0;
 		zbr->znode = NULL;
 
 		/* Validate branch */
@@ -493,12 +493,10 @@ int ubifs_tnc_read_node(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 
 	if (key_type(c, &(zbr->key)) == UBIFS_DATA_KEY) {
 		struct ubifs_data_node *dn = node;
-		long long crypto_lookup = le64_to_cpu(dn->crypto_lookup);
+		long long ksa_pos = le64_to_cpu(dn->ksa_pos);
 
-		if (zbr->crypto_lookup != 0)
-			ubifs_assert(zbr->crypto_lookup == crypto_lookup);
-		else
-			zbr->crypto_lookup = crypto_lookup;
+		ubifs_assert(!zbr->ksa_pos || zbr->ksa_pos == ksa_pos);
+		zbr->ksa_pos = ksa_pos;
 	}
 
 	return 0;
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index e03adcf..55f2ccd 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -539,7 +539,7 @@ struct ubifs_dent_node {
  * struct ubifs_data_node - data node.
  * @ch: common header
  * @key: node key
- * @crypto_lookup: the node's cryptographic key's position in the KSA
+ * @ksa_pos: the node's cryptographic key's position in the KSA
  * @size: uncompressed data size in bytes
  * @compr_type: compression type (%UBIFS_COMPR_NONE, %UBIFS_COMPR_LZO, etc)
  * @padding: reserved for future, zeroes
@@ -551,7 +551,7 @@ struct ubifs_dent_node {
 struct ubifs_data_node {
 	struct ubifs_ch ch;
 	__u8 key[UBIFS_KEY_LEN];
-	__le64 crypto_lookup;
+	__le64 ksa_pos;
 	__le32 size;
 	__le16 compr_type;
 	__u8 padding1[2]; /* Watch 'zero_data_node_unused()' if changing! */
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 4283a51..cf990e2 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -738,7 +738,7 @@ struct ubifs_jhead {
  * @lnum: LEB number of the target node (indexing node or data node)
  * @offs: target node offset within @lnum
  * @len: target node length
- * @crypto_lookup: the node's cryptographic key's position in the KSA
+ * @ksa_pos: the node's cryptographic key's position in the KSA
  */
 struct ubifs_zbranch {
 	union ubifs_key key;
@@ -749,7 +749,7 @@ struct ubifs_zbranch {
 	int lnum;
 	int offs;
 	int len;
-	long long crypto_lookup;
+	long long ksa_pos;
 };
 
 /**
@@ -1577,10 +1577,10 @@ int ubifs_tnc_lookup_nm(struct ubifs_info *c, const union ubifs_key *key,
 int ubifs_tnc_locate(struct ubifs_info *c, const union ubifs_key *key,
 		     void *node, int *lnum, int *offs);
 int ubifs_tnc_add(struct ubifs_info *c, const union ubifs_key *key, int lnum,
-		  int offs, int len, long long crypto_lookup);
+		  int offs, int len, long long ksa_pos);
 int ubifs_tnc_replace(struct ubifs_info *c, const union ubifs_key *key,
 		      int old_lnum, int old_offs, int lnum, int offs, int len,
-		      long long crypto_lookup);
+		      long long ksa_pos);
 int ubifs_tnc_add_nm(struct ubifs_info *c, const union ubifs_key *key,
 		     int lnum, int offs, int len, const struct qstr *nm);
 int ubifs_tnc_remove(struct ubifs_info *c, const union ubifs_key *key);
-- 
1.7.7.6


-- 
Best Regards,
Artem Bityutskiy

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ