| Message ID | 20180704124137.13396-15-s.hauer@pengutronix.de |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | UBIFS authentication support | expand |
Hi Sascha,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc3 next-20180706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sascha-Hauer/UBIFS-authentication-support/20180705-043506
config: arm-aspeed_g4_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
fs/ubifs/log.o: In function `ubifs_log_start_commit':
>> log.c:(.text+0x9ac): undefined reference to `crypto_shash_update'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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@pengutronix.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/gc.c b/fs/ubifs/gc.c > index a03a47cf880d..ac3a3f7c6a6e 100644 > --- a/fs/ubifs/gc.c > +++ b/fs/ubifs/gc.c > @@ -254,7 +254,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb, > snod->type == UBIFS_DATA_NODE || > snod->type == UBIFS_DENT_NODE || > snod->type == UBIFS_XENT_NODE || > - snod->type == UBIFS_TRUN_NODE); > + snod->type == UBIFS_TRUN_NODE || > + snod->type == UBIFS_AUTH_NODE); > > if (snod->type != UBIFS_INO_NODE && > snod->type != UBIFS_DATA_NODE && > 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. > + ubifs_shash_update(c, hash, (void *)node, nodelen); > + > + node += ALIGN(nodelen, 8); > + len -= ALIGN(nodelen, 8); > + } > + > + ubifs_prepare_auth_node(c, node, hash); > +} > + > /** > * write_head - write data to a journal head. > * @c: UBIFS file-system description object > @@ -255,6 +281,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); > > + if (ubifs_authenticated(c)) > + ubifs_hash_nodes(c, buf, len, c->jheads[jhead].log_hash); > + > err = ubifs_wbuf_write_nolock(wbuf, buf, len); > if (err) > return err; > @@ -542,7 +571,9 @@ 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 */ > - len += host_ui->data_len; > + len += ALIGN(host_ui->data_len, 8); Hmm, this change seems unrelated. Why do you need an ALIGN now? > + len += ubifs_auth_node_sz(c); > > dent = kzalloc(len, GFP_NOFS); > if (!dent) > @@ -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. 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. > 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. > 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; > > @@ -745,6 +781,7 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode, > > ubifs_wbuf_add_ino_nolock(&c->jheads[DATAHD].wbuf, key_inum(c, key)); > release_head(c, DATAHD); > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > > err = ubifs_tnc_add(c, key, lnum, offs, dlen, hash); > if (err) > @@ -784,7 +821,8 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) > int err, lnum, offs; > struct ubifs_ino_node *ino; > struct ubifs_inode *ui = ubifs_inode(inode); > - int sync = 0, len = UBIFS_INO_NODE_SZ, last_reference = !inode->i_nlink; > + int sync = 0, len, ilen = UBIFS_INO_NODE_SZ, last_reference = !inode->i_nlink; > + int aligned_ilen; > u8 hash[UBIFS_MAX_HASH_LEN]; > > dbg_jnl("ino %lu, nlink %u", inode->i_ino, inode->i_nlink); > @@ -794,9 +832,14 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) > * need to synchronize the write-buffer either. > */ > if (!last_reference) { > - len += ui->data_len; > + ilen += ui->data_len; > sync = IS_SYNC(inode); > } > + > + aligned_ilen = ALIGN(ilen, 8); > + > + len = aligned_ilen + ubifs_auth_node_sz(c); > + > ino = kmalloc(len, GFP_NOFS); > if (!ino) > return -ENOMEM; > @@ -816,17 +859,20 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) > inode->i_ino); > release_head(c, BASEHD); > > + if (ubifs_authenticated(c)) > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > + > if (last_reference) { > err = ubifs_tnc_remove_ino(c, inode->i_ino); > if (err) > goto out_ro; > ubifs_delete_orphan(c, inode->i_ino); > - err = ubifs_add_dirt(c, lnum, len); > + err = ubifs_add_dirt(c, lnum, ilen); > } else { > union ubifs_key key; > > ino_key_init(c, &key, inode->i_ino); > - err = ubifs_tnc_add(c, &key, lnum, offs, len, hash); > + err = ubifs_tnc_add(c, &key, lnum, offs, ilen, hash); > } > if (err) > goto out_ro; > @@ -955,6 +1001,8 @@ int ubifs_jnl_xrename(struct ubifs_info *c, const struct inode *fst_dir, > if (twoparents) > len += plen; > > + len += ubifs_auth_node_sz(c); > + > dent1 = kzalloc(len, GFP_NOFS); > if (!dent1) > return -ENOMEM; > @@ -1014,6 +1062,9 @@ int ubifs_jnl_xrename(struct ubifs_info *c, const struct inode *fst_dir, > } > release_head(c, BASEHD); > > + if (ubifs_authenticated(c)) > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > + > dent_key_init(c, &key, snd_dir->i_ino, snd_nm); > err = ubifs_tnc_add_nm(c, &key, lnum, offs, dlen1, hash_dent1, snd_nm); > if (err) > @@ -1115,6 +1166,9 @@ int ubifs_jnl_rename(struct ubifs_info *c, const struct inode *old_dir, > len = aligned_dlen1 + aligned_dlen2 + ALIGN(ilen, 8) + ALIGN(plen, 8); > if (move) > len += plen; > + > + len += ubifs_auth_node_sz(c); > + > dent = kzalloc(len, GFP_NOFS); > if (!dent) > return -ENOMEM; > @@ -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()? > dent_key_init(c, &key, new_dir->i_ino, new_nm); > err = ubifs_tnc_add_nm(c, &key, lnum, offs, dlen1, hash_dent1, new_nm); > if (err) > @@ -1356,6 +1413,7 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, > struct ubifs_trun_node *trun; > struct ubifs_data_node *uninitialized_var(dn); > int err, dlen, len, lnum, offs, bit, sz, sync = IS_SYNC(inode); > + int aligned_dlen; > struct ubifs_inode *ui = ubifs_inode(inode); > ino_t inum = inode->i_ino; > unsigned int blk; > @@ -1370,6 +1428,9 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, > > sz = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ + > UBIFS_MAX_DATA_NODE_SZ * WORST_COMPR_FACTOR; > + > + sz += ubifs_auth_node_sz(c); > + > ino = kmalloc(sz, GFP_NOFS); > if (!ino) > return -ENOMEM; > @@ -1404,10 +1465,15 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, > } > } > > + aligned_dlen = ALIGN(dlen, 8); > + > /* Must make reservation before allocating sequence numbers */ > len = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ; > if (dlen) > - len += dlen; > + len += aligned_dlen; > + > + len += ubifs_auth_node_sz(c); > + > err = make_reservation(c, BASEHD, len); > if (err) > goto out_free; > @@ -1428,6 +1494,9 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, > ubifs_wbuf_add_ino_nolock(&c->jheads[BASEHD].wbuf, inum); > release_head(c, BASEHD); > > + if (ubifs_authenticated(c)) > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > + > if (dlen) { > sz = offs + UBIFS_INO_NODE_SZ + UBIFS_TRUN_NODE_SZ; > err = ubifs_tnc_add(c, &key, lnum, sz, dlen, hash_dn); > @@ -1491,7 +1560,7 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host, > const struct inode *inode, > const struct fscrypt_name *nm) > { > - int err, xlen, hlen, len, lnum, xent_offs, aligned_xlen; > + int err, xlen, hlen, len, lnum, xent_offs, aligned_xlen, tlen; > struct ubifs_dent_node *xent; > struct ubifs_ino_node *ino; > union ubifs_key xent_key, key1, key2; > @@ -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. > + xent = kzalloc(tlen, GFP_NOFS); > if (!xent) > return -ENOMEM; > > /* Make reservation before allocating sequence numbers */ > - err = make_reservation(c, BASEHD, len); > + err = make_reservation(c, BASEHD, tlen); > if (err) { > kfree(xent); > return err; > @@ -1539,10 +1610,13 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host, > pack_inode(c, ino, host, 1); > ubifs_node_calc_hash(c, ino, hash); > > - err = write_head(c, BASEHD, xent, len, &lnum, &xent_offs, sync); > + err = write_head(c, BASEHD, xent, tlen, &lnum, &xent_offs, sync); > if (!sync && !err) > ubifs_wbuf_add_ino_nolock(&c->jheads[BASEHD].wbuf, host->i_ino); > release_head(c, BASEHD); > + > + if (ubifs_authenticated(c)) > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > kfree(xent); > if (err) > goto out_ro; > @@ -1605,6 +1679,7 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode, > { > int err, len1, len2, aligned_len, aligned_len1, lnum, offs; > struct ubifs_inode *host_ui = ubifs_inode(host); > + struct ubifs_inode *ui = ubifs_inode(inode); > struct ubifs_ino_node *ino; > union ubifs_key key; > int sync = IS_DIRSYNC(host); > @@ -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? > aligned_len1 = ALIGN(len1, 8); > aligned_len = aligned_len1 + ALIGN(len2, 8); > > + aligned_len += ubifs_auth_node_sz(c); > + > ino = kzalloc(aligned_len, GFP_NOFS); > if (!ino) > return -ENOMEM; > @@ -1646,6 +1723,9 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode, > if (err) > goto out_ro; > > + if (ubifs_authenticated(c)) > + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); > + > ino_key_init(c, &key, host->i_ino); > err = ubifs_tnc_add(c, &key, lnum, offs, len1, hash_host); > if (err) > diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c > index 7cffa120a750..311757b2dc1a 100644 > --- a/fs/ubifs/log.c > +++ b/fs/ubifs/log.c > @@ -236,6 +236,7 @@ int ubifs_add_bud_to_log(struct ubifs_info *c, int jhead, int lnum, int offs) > bud->lnum = lnum; > bud->start = offs; > bud->jhead = jhead; > + bud->log_hash = NULL; > > ref->ch.node_type = UBIFS_REF_NODE; > ref->lnum = cpu_to_le32(bud->lnum); > @@ -275,6 +276,12 @@ int ubifs_add_bud_to_log(struct ubifs_info *c, int jhead, int lnum, int offs) > if (err) > goto out_unlock; > > + ubifs_shash_update(c, c->log_hash, (void *)ref, UBIFS_REF_NODE_SZ); > + > + err = ubifs_shash_copy_state(c, c->log_hash, c->jheads[jhead].log_hash); > + if (err) > + goto out_unlock; > + > c->lhead_offs += c->ref_node_alsz; > > ubifs_add_bud(c, bud); > @@ -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)? > + 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) > + UBIFS_REF_NODE_SZ); > + ubifs_shash_copy_state(c, c->log_hash, c->jheads[i].log_hash); > } > > ubifs_pad(c, buf + len, ALIGN(len, c->min_io_size) - len); > @@ -516,6 +532,7 @@ int ubifs_log_post_commit(struct ubifs_info *c, int old_ltail_lnum) > if (err) > return err; > list_del(&bud->list); > + kfree(bud->log_hash); > kfree(bud); > } > mutex_lock(&c->log_mutex); > diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c > index 9e9ff753515f..07a66ae90e89 100644 > --- a/fs/ubifs/replay.c > +++ b/fs/ubifs/replay.c > @@ -665,6 +665,8 @@ static int replay_bud(struct ubifs_info *c, struct bud_entry *b) > old_size, new_size); > break; > } > + case UBIFS_AUTH_NODE: > + break; > default: > ubifs_err(c, "unexpected node type %d in bud LEB %d:%d", > snod->type, lnum, snod->offs); > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 6b2d80391111..49de06921427 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -815,6 +815,9 @@ static int alloc_wbufs(struct ubifs_info *c) > c->jheads[i].wbuf.sync_callback = &bud_wbuf_callback; > c->jheads[i].wbuf.jhead = i; > c->jheads[i].grouped = 1; > + c->jheads[i].log_hash = ubifs_hash_get_desc(c); > + if (IS_ERR(c->jheads[i].log_hash)) > + goto out; > } > > /* > @@ -825,6 +828,12 @@ static int alloc_wbufs(struct ubifs_info *c) > c->jheads[GCHD].grouped = 0; > > return 0; > + > +out: > + while (i--) > + kfree(c->jheads[i].log_hash); > + > + return err; > } > > /** > @@ -839,6 +848,7 @@ static void free_wbufs(struct ubifs_info *c) > for (i = 0; i < c->jhead_cnt; i++) { > kfree(c->jheads[i].wbuf.buf); > kfree(c->jheads[i].wbuf.inodes); > + kfree(c->jheads[i].log_hash); > } > kfree(c->jheads); > c->jheads = NULL; > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h > index bf4a99d799a1..5390d087da3a 100644 > --- a/fs/ubifs/ubifs.h > +++ b/fs/ubifs/ubifs.h > @@ -697,6 +697,7 @@ struct ubifs_wbuf { > * @jhead: journal head number this bud belongs to > * @list: link in the list buds belonging to the same journal head > * @rb: link in the tree of all buds > + * @log_hash: the log hash from the commit start node up to this bud > */ > struct ubifs_bud { > int lnum; > @@ -704,6 +705,7 @@ struct ubifs_bud { > int jhead; > struct list_head list; > struct rb_node rb; > + struct shash_desc *log_hash; > }; > > /** > @@ -711,6 +713,7 @@ struct ubifs_bud { > * @wbuf: head's write-buffer > * @buds_list: list of bud LEBs belonging to this journal head > * @grouped: non-zero if UBIFS groups nodes when writing to this journal head > + * @log_hash: the log hash from the commit start node up to this journal head > * > * Note, the @buds list is protected by the @c->buds_lock. > */ > @@ -718,6 +721,7 @@ struct ubifs_jhead { > struct ubifs_wbuf wbuf; > struct list_head buds_list; > unsigned int grouped:1; > + struct shash_desc *log_hash; > }; > > /** > @@ -1215,6 +1219,8 @@ struct ubifs_debug_info; > * @auth_key_name: the authentication key name > * @auth_hash_name: the name of the hash algorithm used for authentication > * @auth_hash_algo: the authentication hash used for this fs > + * @log_hash: the log hash from the commit start node up to the latest reference > + * node. > * > * @empty: %1 if the UBI device is empty > * @need_recovery: %1 if the file-system needs recovery > @@ -1456,6 +1462,8 @@ struct ubifs_info { > char *auth_hash_name; > enum hash_algo auth_hash_algo; > > + struct shash_desc *log_hash; > + > /* The below fields are used only during mounting and re-mounting */ > unsigned int empty:1; > unsigned int need_recovery:1; > Thanks, //richard
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@pengutronix.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
Am Mittwoch, 29. August 2018, 16:38:34 CEST schrieb Sascha Hauer: > On Mon, Aug 27, 2018 at 10:48:26PM +0200, Richard Weinberger wrote: > > > 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? I don't fully understand how you keep the lprops dirty counter correct for auth nodes. Hence the question. Auth nodes are not referenced by the index, so you have to keep track of them manually. Is your current approach "mark them dirty immediately and rely on LPT commit" to not get lost of an auth node? I expected auth nodes getting dirtied also during journal reply. > > > > 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? Uhh. Let me check this. More corner cases, I fear. > > > @@ -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. write_len is very good! Thanks, //richard
On Wed, Aug 29, 2018 at 04:54:30PM +0200, Richard Weinberger wrote: > Am Mittwoch, 29. August 2018, 16:38:34 CEST schrieb Sascha Hauer: > > On Mon, Aug 27, 2018 at 10:48:26PM +0200, Richard Weinberger wrote: > > > > 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? > > I don't fully understand how you keep the lprops dirty counter correct for > auth nodes. Hence the question. > > Auth nodes are not referenced by the index, so you have to keep track of > them manually. Yes. That's why I call ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)) each time I create an auth node. > Is your current approach "mark them dirty immediately and rely on LPT commit" > to not get lost of an auth node? Yes, I mark the auth nodes dirty in the assumption that the correct values are written on a LPT commit. > > I expected auth nodes getting dirtied also during journal reply. Yes, this happens in replay.c replay_bud(). "used" gets increased with every node found except when it's an auth node. Here "used" is not increased, the result is that the auth node adds to the dirty space. Sascha
Am Mittwoch, 29. August 2018, 16:38:34 CEST schrieb Sascha Hauer: > 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? Well, this is not the only call site. Other call sites do so implicitly since most UBIFS nodes sizes are aligned to 8. But yes, there are some inconsistencies which need a cleanup. I've scheduled that for the upcoming week. Thanks, //richard
diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c index a03a47cf880d..ac3a3f7c6a6e 100644 --- a/fs/ubifs/gc.c +++ b/fs/ubifs/gc.c @@ -254,7 +254,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb, snod->type == UBIFS_DATA_NODE || snod->type == UBIFS_DENT_NODE || snod->type == UBIFS_XENT_NODE || - snod->type == UBIFS_TRUN_NODE); + snod->type == UBIFS_TRUN_NODE || + snod->type == UBIFS_AUTH_NODE); if (snod->type != UBIFS_INO_NODE && snod->type != UBIFS_DATA_NODE && 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)); + + ubifs_shash_update(c, hash, (void *)node, nodelen); + + node += ALIGN(nodelen, 8); + len -= ALIGN(nodelen, 8); + } + + ubifs_prepare_auth_node(c, node, hash); +} + /** * write_head - write data to a journal head. * @c: UBIFS file-system description object @@ -255,6 +281,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); + if (ubifs_authenticated(c)) + ubifs_hash_nodes(c, buf, len, c->jheads[jhead].log_hash); + err = ubifs_wbuf_write_nolock(wbuf, buf, len); if (err) return err; @@ -542,7 +571,9 @@ 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 */ - len += host_ui->data_len; + len += ALIGN(host_ui->data_len, 8); + + len += ubifs_auth_node_sz(c); dent = kzalloc(len, GFP_NOFS); if (!dent) @@ -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)); 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); 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; @@ -745,6 +781,7 @@ int ubifs_jnl_write_data(struct ubifs_info *c, const struct inode *inode, ubifs_wbuf_add_ino_nolock(&c->jheads[DATAHD].wbuf, key_inum(c, key)); release_head(c, DATAHD); + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); err = ubifs_tnc_add(c, key, lnum, offs, dlen, hash); if (err) @@ -784,7 +821,8 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) int err, lnum, offs; struct ubifs_ino_node *ino; struct ubifs_inode *ui = ubifs_inode(inode); - int sync = 0, len = UBIFS_INO_NODE_SZ, last_reference = !inode->i_nlink; + int sync = 0, len, ilen = UBIFS_INO_NODE_SZ, last_reference = !inode->i_nlink; + int aligned_ilen; u8 hash[UBIFS_MAX_HASH_LEN]; dbg_jnl("ino %lu, nlink %u", inode->i_ino, inode->i_nlink); @@ -794,9 +832,14 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) * need to synchronize the write-buffer either. */ if (!last_reference) { - len += ui->data_len; + ilen += ui->data_len; sync = IS_SYNC(inode); } + + aligned_ilen = ALIGN(ilen, 8); + + len = aligned_ilen + ubifs_auth_node_sz(c); + ino = kmalloc(len, GFP_NOFS); if (!ino) return -ENOMEM; @@ -816,17 +859,20 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) inode->i_ino); release_head(c, BASEHD); + if (ubifs_authenticated(c)) + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); + if (last_reference) { err = ubifs_tnc_remove_ino(c, inode->i_ino); if (err) goto out_ro; ubifs_delete_orphan(c, inode->i_ino); - err = ubifs_add_dirt(c, lnum, len); + err = ubifs_add_dirt(c, lnum, ilen); } else { union ubifs_key key; ino_key_init(c, &key, inode->i_ino); - err = ubifs_tnc_add(c, &key, lnum, offs, len, hash); + err = ubifs_tnc_add(c, &key, lnum, offs, ilen, hash); } if (err) goto out_ro; @@ -955,6 +1001,8 @@ int ubifs_jnl_xrename(struct ubifs_info *c, const struct inode *fst_dir, if (twoparents) len += plen; + len += ubifs_auth_node_sz(c); + dent1 = kzalloc(len, GFP_NOFS); if (!dent1) return -ENOMEM; @@ -1014,6 +1062,9 @@ int ubifs_jnl_xrename(struct ubifs_info *c, const struct inode *fst_dir, } release_head(c, BASEHD); + if (ubifs_authenticated(c)) + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); + dent_key_init(c, &key, snd_dir->i_ino, snd_nm); err = ubifs_tnc_add_nm(c, &key, lnum, offs, dlen1, hash_dent1, snd_nm); if (err) @@ -1115,6 +1166,9 @@ int ubifs_jnl_rename(struct ubifs_info *c, const struct inode *old_dir, len = aligned_dlen1 + aligned_dlen2 + ALIGN(ilen, 8) + ALIGN(plen, 8); if (move) len += plen; + + len += ubifs_auth_node_sz(c); + dent = kzalloc(len, GFP_NOFS); if (!dent) return -ENOMEM; @@ -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)); + dent_key_init(c, &key, new_dir->i_ino, new_nm); err = ubifs_tnc_add_nm(c, &key, lnum, offs, dlen1, hash_dent1, new_nm); if (err) @@ -1356,6 +1413,7 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, struct ubifs_trun_node *trun; struct ubifs_data_node *uninitialized_var(dn); int err, dlen, len, lnum, offs, bit, sz, sync = IS_SYNC(inode); + int aligned_dlen; struct ubifs_inode *ui = ubifs_inode(inode); ino_t inum = inode->i_ino; unsigned int blk; @@ -1370,6 +1428,9 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, sz = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ + UBIFS_MAX_DATA_NODE_SZ * WORST_COMPR_FACTOR; + + sz += ubifs_auth_node_sz(c); + ino = kmalloc(sz, GFP_NOFS); if (!ino) return -ENOMEM; @@ -1404,10 +1465,15 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, } } + aligned_dlen = ALIGN(dlen, 8); + /* Must make reservation before allocating sequence numbers */ len = UBIFS_TRUN_NODE_SZ + UBIFS_INO_NODE_SZ; if (dlen) - len += dlen; + len += aligned_dlen; + + len += ubifs_auth_node_sz(c); + err = make_reservation(c, BASEHD, len); if (err) goto out_free; @@ -1428,6 +1494,9 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, ubifs_wbuf_add_ino_nolock(&c->jheads[BASEHD].wbuf, inum); release_head(c, BASEHD); + if (ubifs_authenticated(c)) + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); + if (dlen) { sz = offs + UBIFS_INO_NODE_SZ + UBIFS_TRUN_NODE_SZ; err = ubifs_tnc_add(c, &key, lnum, sz, dlen, hash_dn); @@ -1491,7 +1560,7 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host, const struct inode *inode, const struct fscrypt_name *nm) { - int err, xlen, hlen, len, lnum, xent_offs, aligned_xlen; + int err, xlen, hlen, len, lnum, xent_offs, aligned_xlen, tlen; struct ubifs_dent_node *xent; struct ubifs_ino_node *ino; union ubifs_key xent_key, key1, key2; @@ -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); + + xent = kzalloc(tlen, GFP_NOFS); if (!xent) return -ENOMEM; /* Make reservation before allocating sequence numbers */ - err = make_reservation(c, BASEHD, len); + err = make_reservation(c, BASEHD, tlen); if (err) { kfree(xent); return err; @@ -1539,10 +1610,13 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host, pack_inode(c, ino, host, 1); ubifs_node_calc_hash(c, ino, hash); - err = write_head(c, BASEHD, xent, len, &lnum, &xent_offs, sync); + err = write_head(c, BASEHD, xent, tlen, &lnum, &xent_offs, sync); if (!sync && !err) ubifs_wbuf_add_ino_nolock(&c->jheads[BASEHD].wbuf, host->i_ino); release_head(c, BASEHD); + + if (ubifs_authenticated(c)) + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); kfree(xent); if (err) goto out_ro; @@ -1605,6 +1679,7 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode, { int err, len1, len2, aligned_len, aligned_len1, lnum, offs; struct ubifs_inode *host_ui = ubifs_inode(host); + struct ubifs_inode *ui = ubifs_inode(inode); struct ubifs_ino_node *ino; union ubifs_key key; int sync = IS_DIRSYNC(host); @@ -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; aligned_len1 = ALIGN(len1, 8); aligned_len = aligned_len1 + ALIGN(len2, 8); + aligned_len += ubifs_auth_node_sz(c); + ino = kzalloc(aligned_len, GFP_NOFS); if (!ino) return -ENOMEM; @@ -1646,6 +1723,9 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode, if (err) goto out_ro; + if (ubifs_authenticated(c)) + ubifs_add_dirt(c, lnum, ubifs_auth_node_sz(c)); + ino_key_init(c, &key, host->i_ino); err = ubifs_tnc_add(c, &key, lnum, offs, len1, hash_host); if (err) diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c index 7cffa120a750..311757b2dc1a 100644 --- a/fs/ubifs/log.c +++ b/fs/ubifs/log.c @@ -236,6 +236,7 @@ int ubifs_add_bud_to_log(struct ubifs_info *c, int jhead, int lnum, int offs) bud->lnum = lnum; bud->start = offs; bud->jhead = jhead; + bud->log_hash = NULL; ref->ch.node_type = UBIFS_REF_NODE; ref->lnum = cpu_to_le32(bud->lnum); @@ -275,6 +276,12 @@ int ubifs_add_bud_to_log(struct ubifs_info *c, int jhead, int lnum, int offs) if (err) goto out_unlock; + ubifs_shash_update(c, c->log_hash, (void *)ref, UBIFS_REF_NODE_SZ); + + err = ubifs_shash_copy_state(c, c->log_hash, c->jheads[jhead].log_hash); + if (err) + goto out_unlock; + c->lhead_offs += c->ref_node_alsz; ubifs_add_bud(c, bud); @@ -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) { + 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, + UBIFS_REF_NODE_SZ); + ubifs_shash_copy_state(c, c->log_hash, c->jheads[i].log_hash); } ubifs_pad(c, buf + len, ALIGN(len, c->min_io_size) - len); @@ -516,6 +532,7 @@ int ubifs_log_post_commit(struct ubifs_info *c, int old_ltail_lnum) if (err) return err; list_del(&bud->list); + kfree(bud->log_hash); kfree(bud); } mutex_lock(&c->log_mutex); diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index 9e9ff753515f..07a66ae90e89 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -665,6 +665,8 @@ static int replay_bud(struct ubifs_info *c, struct bud_entry *b) old_size, new_size); break; } + case UBIFS_AUTH_NODE: + break; default: ubifs_err(c, "unexpected node type %d in bud LEB %d:%d", snod->type, lnum, snod->offs); diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 6b2d80391111..49de06921427 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -815,6 +815,9 @@ static int alloc_wbufs(struct ubifs_info *c) c->jheads[i].wbuf.sync_callback = &bud_wbuf_callback; c->jheads[i].wbuf.jhead = i; c->jheads[i].grouped = 1; + c->jheads[i].log_hash = ubifs_hash_get_desc(c); + if (IS_ERR(c->jheads[i].log_hash)) + goto out; } /* @@ -825,6 +828,12 @@ static int alloc_wbufs(struct ubifs_info *c) c->jheads[GCHD].grouped = 0; return 0; + +out: + while (i--) + kfree(c->jheads[i].log_hash); + + return err; } /** @@ -839,6 +848,7 @@ static void free_wbufs(struct ubifs_info *c) for (i = 0; i < c->jhead_cnt; i++) { kfree(c->jheads[i].wbuf.buf); kfree(c->jheads[i].wbuf.inodes); + kfree(c->jheads[i].log_hash); } kfree(c->jheads); c->jheads = NULL; diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index bf4a99d799a1..5390d087da3a 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -697,6 +697,7 @@ struct ubifs_wbuf { * @jhead: journal head number this bud belongs to * @list: link in the list buds belonging to the same journal head * @rb: link in the tree of all buds + * @log_hash: the log hash from the commit start node up to this bud */ struct ubifs_bud { int lnum; @@ -704,6 +705,7 @@ struct ubifs_bud { int jhead; struct list_head list; struct rb_node rb; + struct shash_desc *log_hash; }; /** @@ -711,6 +713,7 @@ struct ubifs_bud { * @wbuf: head's write-buffer * @buds_list: list of bud LEBs belonging to this journal head * @grouped: non-zero if UBIFS groups nodes when writing to this journal head + * @log_hash: the log hash from the commit start node up to this journal head * * Note, the @buds list is protected by the @c->buds_lock. */ @@ -718,6 +721,7 @@ struct ubifs_jhead { struct ubifs_wbuf wbuf; struct list_head buds_list; unsigned int grouped:1; + struct shash_desc *log_hash; }; /** @@ -1215,6 +1219,8 @@ struct ubifs_debug_info; * @auth_key_name: the authentication key name * @auth_hash_name: the name of the hash algorithm used for authentication * @auth_hash_algo: the authentication hash used for this fs + * @log_hash: the log hash from the commit start node up to the latest reference + * node. * * @empty: %1 if the UBI device is empty * @need_recovery: %1 if the file-system needs recovery @@ -1456,6 +1462,8 @@ struct ubifs_info { char *auth_hash_name; enum hash_algo auth_hash_algo; + struct shash_desc *log_hash; + /* The below fields are used only during mounting and re-mounting */ unsigned int empty:1; unsigned int need_recovery:1;
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@pengutronix.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(-)