Date:   Wed, 29 Aug 2018 16:38:34 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Richard Weinberger <richard@....at>
Cc:     linux-mtd@...ts.infradead.org, David Gstir <david@...ma-star.at>,
        kernel@...gutronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 14/25] ubifs: Add authentication nodes to journal

On Mon, Aug 27, 2018 at 10:48:26PM +0200, Richard Weinberger wrote:
> Am Mittwoch, 4. Juli 2018, 14:41:26 CEST schrieb Sascha Hauer:
> > Nodes that are written to flash can only be authenticated through the
> > index after the next commit. When a journal replay is necessary the
> > nodes are not yet referenced by the index and thus can't be
> > authenticated.
> > 
> > This patch overcomes this situation by creating a hash over all nodes
> > beginning from the commit start node over the reference node(s) and
> > the buds themselves. From
> > time to time we insert authentication nodes. Authentication nodes
> > contain a HMAC from the current hash state, so that they can be
> > used to authenticate a journal replay up to the point where the
> > authentication node is. The hash is continued afterwards
> > so that theoretically we would only have to check the HMAC of
> > the last authentication node we find.
> > 
> > Overall we get this picture:
> > 
> > ,,,,,,,,
> > ,......,...........................................
> > ,. CS  ,               hash1.----.           hash2.----.
> > ,.  |  ,                    .    |hmac            .    |hmac
> > ,.  v  ,                    .    v                .    v
> > ,.REF#0,-> bud -> bud -> bud.-> auth -> bud -> bud.-> auth ...
> > ,..|...,...........................................
> > ,  |   ,
> > ,  |   ,,,,,,,,,,,,,,,
> > .  |            hash3,----.
> > ,  |                 ,    |hmac
> > ,  v                 ,    v
> > , REF#1 -> bud -> bud,-> auth ...
> > ,,,|,,,,,,,,,,,,,,,,,,
> >    v
> >   REF#2 -> ...
> >    |
> >    V
> >   ...
> > 
> > Note how hash3 covers CS, REF#0 and REF#1 so that it is not possible to
> > exchange or skip any reference nodes. Unlike the picture suggests the
> > auth nodes themselves are not hashed.
> > 
> > With this it is possible for an offline attacker to cut each journal
> > head or to drop the last reference node(s), but not to skip any journal
> > heads or to reorder any operations.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> > ---
> >  fs/ubifs/gc.c      |   3 +-
> >  fs/ubifs/journal.c | 110 ++++++++++++++++++++++++++++++++++++++-------
> >  fs/ubifs/log.c     |  17 +++++++
> >  fs/ubifs/replay.c  |   2 +
> >  fs/ubifs/super.c   |  10 +++++
> >  fs/ubifs/ubifs.h   |   8 ++++
> >  6 files changed, 134 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> > index 55b35bc33c31..4fde75623a3f 100644
> > --- a/fs/ubifs/journal.c
> > +++ b/fs/ubifs/journal.c
> > @@ -228,6 +228,32 @@ static int reserve_space(struct ubifs_info *c, int jhead, int len)
> >  	return err;
> >  }
> >  
> > +static void ubifs_hash_nodes(struct ubifs_info *c, void *node,
> > +			     int len, struct shash_desc *hash)
> > +{
> > +	int auth_node_size = ubifs_auth_node_sz(c);
> > +
> > +	while (1) {
> > +		const struct ubifs_ch *ch = node;
> > +		int nodelen = le32_to_cpu(ch->len);
> > +
> > +		ubifs_assert(len >= auth_node_size);
> > +
> > +		if (len == auth_node_size)
> > +			break;
> > +
> > +		ubifs_assert(len > nodelen);
> > +		ubifs_assert(ch->magic == cpu_to_le32(UBIFS_NODE_MAGIC));
> 
> A malformed UBIFS image can trigger that assert, right?
> Please handle this without ubifs_assert() and abort with an error.
> ubifs_assert() does not stop execution by default.

In this function we iterate over the nodes we previously created in
memory. It is called with the same buffer write_head() is called with.

If this assertion triggers then we either failed creating a good buffer
containing all nodes or we failed iterating over it for some reason.
Either way it is an UBIFS internal error, not a malformed image.

> 
> > +		ubifs_shash_update(c, hash, (void *)node, nodelen);
> > +
> > +		node += ALIGN(nodelen, 8);
> > +		len -= ALIGN(nodelen, 8);
> > +	}
> > +
> > +	ubifs_prepare_auth_node(c, node, hash);
> > +}
> > +
> > @@ -603,6 +634,7 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
> >  	}
> >  	release_head(c, BASEHD);
> >  	kfree(dent);
> > +	ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c));
> 
> You have to account it immediately because while a commit you have no longer
> a reference to them?
> Upon replay you should have since you scan LEBs anyway.

What do you mean here? Is that a suggestion to change something?

> 
> An shouldn't this only get called when the file system is authenticated?
> AFAICT ubifs_add_dirt(c, lnum, 0) is not a no-op.

Right. I changed it to use the ubifs_add_auth_dirt() helper that you
suggested below.

> 
> >  	if (deletion) {
> >  		if (nm->hash)
> > @@ -677,8 +709,9 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
> >  			 const union ubifs_key *key, const void *buf, int len)
> >  {
> >  	struct ubifs_data_node *data;
> > -	int err, lnum, offs, compr_type, out_len, compr_len;
> > +	int err, lnum, offs, compr_type, out_len, compr_len, auth_len;
> >  	int dlen = COMPRESSED_DATA_NODE_BUF_SZ, allocated = 1;
> > +	int aligned_dlen;
> >  	struct ubifs_inode *ui = ubifs_inode(inode);
> >  	bool encrypted = ubifs_crypt_is_encrypted(inode);
> >  	u8 hash[UBIFS_MAX_HASH_LEN];
> > @@ -690,7 +723,9 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
> >  	if (encrypted)
> >  		dlen += UBIFS_CIPHER_BLOCK_SIZE;
> >  
> > -	data = kmalloc(dlen, GFP_NOFS | __GFP_NOWARN);
> > +	auth_len = ubifs_auth_node_sz(c);
> > +
> > +	data = kmalloc(dlen + auth_len, GFP_NOFS | __GFP_NOWARN);
> >  	if (!data) {
> >  		/*
> >  		 * Fall-back to the write reserve buffer. Note, we might be
> > @@ -729,15 +764,16 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode,
> >  	}
> >  
> >  	dlen = UBIFS_DATA_NODE_SZ + out_len;
> > +	aligned_dlen = ALIGN(dlen, 8);
> >  	data->compr_type = cpu_to_le16(compr_type);
> >  
> >  	/* Make reservation before allocating sequence numbers */
> > -	err = make_reservation(c, DATAHD, dlen);
> > +	err = make_reservation(c, DATAHD, aligned_dlen + auth_len);
> 
> Okay, now I understand the ALIGN(), ubifs nodes need to be aligned
> at an 8 border. Makes sense, _but_ you change this also for the non-authenticated
> case.

I assumed that make_reservation would align len anyway. I can't find the
place that led me to that assumption anymore and even if this is true
it's probably safer to just stick to the original len for the
non-authenticated case, so I'll change this and other places to use
the non aligned len.

BTW could you have a look at ubifs_jnl_change_xattr()? Unlike other
function in this file it explicitly calls make_reservation() with the
length of the last node aligned. Do you have an idea why?

> 
> >  	if (err)
> >  		goto out_free;
> >  
> >  	ubifs_prepare_node(c, data, dlen, 0);
> > -	err = write_head(c, DATAHD, data, dlen, &lnum, &offs, 0);
> > +	err = write_head(c, DATAHD, data, aligned_dlen + auth_len, &lnum, &offs, 0);
> >  	if (err)
> >  		goto out_release;
> >  
> > @@ -1198,6 +1252,9 @@ int ubifs_jnl_rename(struct ubifs_info *c, const struct inode *old_dir,
> >  	}
> >  	release_head(c, BASEHD);
> >  
> > +	if (ubifs_authenticated(c))
> > +		ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c));
> 
> This is always the same pattern. How about adding a helper function?
> ubifs_add_auth_dirt()?

Yes, sounds good.

> > @@ -1511,12 +1580,14 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host,
> >  	hlen = host_ui->data_len + UBIFS_INO_NODE_SZ;
> >  	len = aligned_xlen + UBIFS_INO_NODE_SZ + ALIGN(hlen, 8);
> >  
> > -	xent = kzalloc(len, GFP_NOFS);
> > +	tlen = len + ubifs_auth_node_sz(c);
> 
> xlen, hlen, len, tlen, oh my.. ;-)
> What does the "t" stand for?
> Sorry, I'm very bad at naming things.

I must have thought of something like total_len. I could change it to
write_len if that sounds better to you.

> > @@ -1617,10 +1692,12 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode,
> >  	ubifs_assert(mutex_is_locked(&host_ui->ui_mutex));
> >  
> >  	len1 = UBIFS_INO_NODE_SZ + host_ui->data_len;
> > -	len2 = UBIFS_INO_NODE_SZ + ubifs_inode(inode)->data_len;
> > +	len2 = UBIFS_INO_NODE_SZ + ui->data_len;
> 
> Why do we need this change, seems unrelated?

Some leftover from earlier versions. Will drop.

> > @@ -377,6 +384,11 @@ int ubifs_log_start_commit(struct ubifs_info *c, int *ltail_lnum)
> >  	cs->cmt_no = cpu_to_le64(c->cmt_no);
> >  	ubifs_prepare_node(c, cs, UBIFS_CS_NODE_SZ, 0);
> >  
> > +	if (c->authenticated) {
> 
> ubifs_authenticated(c)?

Yes.

> 
> > +		crypto_shash_init(c->log_hash);
> > +		crypto_shash_update(c->log_hash, (void *)cs, UBIFS_CS_NODE_SZ);
> > +	}
> > +
> >  	/*
> >  	 * Note, we do not lock 'c->log_mutex' because this is the commit start
> >  	 * phase and we are exclusively using the log. And we do not lock
> > @@ -402,6 +414,10 @@ int ubifs_log_start_commit(struct ubifs_info *c, int *ltail_lnum)
> >  
> >  		ubifs_prepare_node(c, ref, UBIFS_REF_NODE_SZ, 0);
> >  		len += UBIFS_REF_NODE_SZ;
> > +
> > +		ubifs_shash_update(c, c->log_hash, (void *)ref,
> 
> Why the void * cast?
> (Applies to multiple calls to ubifs_shash_update)

I think I used crypto_shash_update directly in ealier versions which
takes a u8 * and thus needs a cast. Now it can be removed.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists