From eb66eff6636de0e83f74294447b47aec51e069fc Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 2 Nov 2018 16:39:24 +0100 Subject: [PATCH 1/8] ubifs: replay: Fix high stack usage Having two shash descriptors on the stack cause a very significant kernel stack usage that can cross the warning threshold: fs/ubifs/replay.c: In function 'authenticate_sleb': fs/ubifs/replay.c:633:1: error: the frame size of 1144 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] Normally, gcc optimizes the out, but with CONFIG_CC_OPTIMIZE_FOR_DEBUGGING, it does not. Splitting the two stack allocations into separate functions means that they will use the same memory again. In normal configurations (optimizing for size or performance), those should get inlined and we get the same behavior as before. Signed-off-by: Arnd Bergmann Signed-off-by: Richard Weinberger --- fs/ubifs/replay.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index 75f961c4c044..a08c5b7030ea 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -533,6 +533,28 @@ static int is_last_bud(struct ubifs_info *c, struct ubifs_bud *bud) return data == 0xFFFFFFFF; } +/* authenticate_sleb_hash and authenticate_sleb_hmac are split out for stack usage */ +static int authenticate_sleb_hash(struct ubifs_info *c, struct shash_desc *log_hash, u8 *hash) +{ + SHASH_DESC_ON_STACK(hash_desc, c->hash_tfm); + + hash_desc->tfm = c->hash_tfm; + hash_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; + + ubifs_shash_copy_state(c, log_hash, hash_desc); + return crypto_shash_final(hash_desc, hash); +} + +static int authenticate_sleb_hmac(struct ubifs_info *c, u8 *hash, u8 *hmac) +{ + SHASH_DESC_ON_STACK(hmac_desc, c->hmac_tfm); + + hmac_desc->tfm = c->hmac_tfm; + hmac_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; + + return crypto_shash_digest(hmac_desc, hash, c->hash_len, hmac); +} + /** * authenticate_sleb - authenticate one scan LEB * @c: UBIFS file-system description object @@ -574,21 +596,12 @@ static int authenticate_sleb(struct ubifs_info *c, struct ubifs_scan_leb *sleb, if (snod->type == UBIFS_AUTH_NODE) { struct ubifs_auth_node *auth = snod->node; - SHASH_DESC_ON_STACK(hash_desc, c->hash_tfm); - SHASH_DESC_ON_STACK(hmac_desc, c->hmac_tfm); - hash_desc->tfm = c->hash_tfm; - hash_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; - - ubifs_shash_copy_state(c, log_hash, hash_desc); - err = crypto_shash_final(hash_desc, hash); + err = authenticate_sleb_hash(c, log_hash, hash); if (err) goto out; - hmac_desc->tfm = c->hmac_tfm; - hmac_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; - err = crypto_shash_digest(hmac_desc, hash, c->hash_len, - hmac); + err = authenticate_sleb_hmac(c, hash, hmac); if (err) goto out; From aa3d31e08c5707c99feebe53cb713fb43f526f16 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Mon, 5 Nov 2018 09:25:40 +0100 Subject: [PATCH 2/8] ubifs: CONFIG_UBIFS_FS_AUTHENTICATION should depend on UBIFS_FS Instead of adding yet another dependency on UBIFS_FS, wrap the whole block of ubifs config options in a single "if UBIFS_FS". Fixes: d8a22773a12c6d78 ("ubifs: Enable authentication support") Signed-off-by: Geert Uytterhoeven Acked-by: Sascha Hauer Signed-off-by: Richard Weinberger --- fs/ubifs/Kconfig | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig index 529856fbccd0..3ee613361f4e 100644 --- a/fs/ubifs/Kconfig +++ b/fs/ubifs/Kconfig @@ -12,9 +12,10 @@ config UBIFS_FS help UBIFS is a file system for flash devices which works on top of UBI. +if UBIFS_FS + config UBIFS_FS_ADVANCED_COMPR bool "Advanced compression options" - depends on UBIFS_FS help This option allows to explicitly choose which compressions, if any, are enabled in UBIFS. Removing compressors means inability to read @@ -24,7 +25,6 @@ config UBIFS_FS_ADVANCED_COMPR config UBIFS_FS_LZO bool "LZO compression support" if UBIFS_FS_ADVANCED_COMPR - depends on UBIFS_FS default y help LZO compressor is generally faster than zlib but compresses worse. @@ -32,14 +32,12 @@ config UBIFS_FS_LZO config UBIFS_FS_ZLIB bool "ZLIB compression support" if UBIFS_FS_ADVANCED_COMPR - depends on UBIFS_FS default y help Zlib compresses better than LZO but it is slower. Say 'Y' if unsure. config UBIFS_ATIME_SUPPORT - bool "Access time support" if UBIFS_FS - depends on UBIFS_FS + bool "Access time support" default n help Originally UBIFS did not support atime, because it looked like a bad idea due @@ -54,7 +52,6 @@ config UBIFS_ATIME_SUPPORT config UBIFS_FS_XATTR bool "UBIFS XATTR support" - depends on UBIFS_FS default y help Saying Y here includes support for extended attributes (xattrs). @@ -65,7 +62,7 @@ config UBIFS_FS_XATTR config UBIFS_FS_ENCRYPTION bool "UBIFS Encryption" - depends on UBIFS_FS && UBIFS_FS_XATTR && BLOCK + depends on UBIFS_FS_XATTR && BLOCK select FS_ENCRYPTION default n help @@ -76,7 +73,7 @@ config UBIFS_FS_ENCRYPTION config UBIFS_FS_SECURITY bool "UBIFS Security Labels" - depends on UBIFS_FS && UBIFS_FS_XATTR + depends on UBIFS_FS_XATTR default y help Security labels provide an access control facility to support Linux @@ -96,3 +93,5 @@ config UBIFS_FS_AUTHENTICATION If you say yes here you should also select a hashing algorithm such as sha256, these are not selected automatically since there are many different options. + +endif # UBIFS_FS From 60eb5da2434b5288291aac533fb4dadc8ec8aed7 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 2 Nov 2018 12:11:22 +0100 Subject: [PATCH 3/8] ubifs: auth: Add CONFIG_KEYS dependency The new authentication support causes a build failure when CONFIG_KEYS is disabled, so add a dependency. fs/ubifs/auth.c: In function 'ubifs_init_authentication': fs/ubifs/auth.c:249:16: error: implicit declaration of function 'request_key'; did you mean 'request_irq'? [-Werror=implicit-function-declaration] keyring_key = request_key(&key_type_logon, c->auth_key_name, NULL); Fixes: d8a22773a12c ("ubifs: Enable authentication support") Signed-off-by: Arnd Bergmann Signed-off-by: Richard Weinberger --- fs/ubifs/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig index 3ee613361f4e..bc1e082d921d 100644 --- a/fs/ubifs/Kconfig +++ b/fs/ubifs/Kconfig @@ -86,6 +86,7 @@ config UBIFS_FS_SECURITY config UBIFS_FS_AUTHENTICATION bool "UBIFS authentication support" + depends on KEYS select CRYPTO_HMAC help Enable authentication support for UBIFS. This feature offers protection From 6554a56f352ee2a6ce295fcfb39889eb9ecc79cf Mon Sep 17 00:00:00 2001 From: Garry McNulty Date: Thu, 15 Nov 2018 22:25:58 +0000 Subject: [PATCH 4/8] ubifs: Fix memory leak on error condition If the call to ubifs_read_nnode() fails in ubifs_lpt_calc_hash() an error is returned without freeing the memory allocated to 'buf'. Read and check the root node before allocating the buffer. Detected by CoverityScan, CID 1441025 ("Resource leak") Signed-off-by: Garry McNulty Signed-off-by: Richard Weinberger --- fs/ubifs/lpt.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c index d1d5e96350dd..b0c5f06128b5 100644 --- a/fs/ubifs/lpt.c +++ b/fs/ubifs/lpt.c @@ -1675,6 +1675,12 @@ int ubifs_lpt_calc_hash(struct ubifs_info *c, u8 *hash) if (!ubifs_authenticated(c)) return 0; + if (!c->nroot) { + err = ubifs_read_nnode(c, NULL, 0); + if (err) + return err; + } + desc = ubifs_hash_get_desc(c); if (IS_ERR(desc)) return PTR_ERR(desc); @@ -1685,12 +1691,6 @@ int ubifs_lpt_calc_hash(struct ubifs_info *c, u8 *hash) goto out; } - if (!c->nroot) { - err = ubifs_read_nnode(c, NULL, 0); - if (err) - return err; - } - cnode = (struct ubifs_cnode *)c->nroot; while (cnode) { From d62e98ed1efcaa94caa004f622944afdce5f1c3c Mon Sep 17 00:00:00 2001 From: Gabor Juhos Date: Sun, 9 Dec 2018 18:12:13 +0100 Subject: [PATCH 5/8] ubifs: Fix default compression selection in ubifs When ubifs is build without the LZO compressor and no compressor is given the creation of the default file system will fail. before selection the LZO compressor check if it is present and if not fall back to the zlib or none. Signed-off-by: Gabor Juhos Signed-off-by: Hauke Mehrtens Signed-off-by: Richard Weinberger --- fs/ubifs/sb.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c index 75a69dd26d6e..3da90c951c23 100644 --- a/fs/ubifs/sb.c +++ b/fs/ubifs/sb.c @@ -63,6 +63,17 @@ /* Default time granularity in nanoseconds */ #define DEFAULT_TIME_GRAN 1000000000 +static int get_default_compressor(struct ubifs_info *c) +{ + if (ubifs_compr_present(c, UBIFS_COMPR_LZO)) + return UBIFS_COMPR_LZO; + + if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB)) + return UBIFS_COMPR_ZLIB; + + return UBIFS_COMPR_NONE; +} + /** * create_default_filesystem - format empty UBI volume. * @c: UBIFS file-system description object @@ -207,7 +218,7 @@ static int create_default_filesystem(struct ubifs_info *c) if (c->mount_opts.override_compr) sup->default_compr = cpu_to_le16(c->mount_opts.compr_type); else - sup->default_compr = cpu_to_le16(UBIFS_COMPR_LZO); + sup->default_compr = cpu_to_le16(get_default_compressor(c)); generate_random_uuid(sup->uuid); From b95f83ab762dd6211351b9140f99f43644076ca8 Mon Sep 17 00:00:00 2001 From: Pan Bian Date: Wed, 28 Nov 2018 10:57:33 +0800 Subject: [PATCH 6/8] ubi: Put MTD device after it is not used The MTD device reference is dropped via put_mtd_device, however its field ->index is read and passed to ubi_msg. To fix this, the patch moves the reference dropping after calling ubi_msg. Signed-off-by: Pan Bian Reviewed-by: Boris Brezillon Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index a4e3454133a4..09170b707339 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1101,10 +1101,10 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) ubi_wl_close(ubi); ubi_free_internal_volumes(ubi); vfree(ubi->vtbl); - put_mtd_device(ubi->mtd); vfree(ubi->peb_buf); vfree(ubi->fm_buf); ubi_msg(ubi, "mtd%d is detached", ubi->mtd->index); + put_mtd_device(ubi->mtd); put_device(&ubi->dev); return 0; } From e542087701f09418702673631a908429feb3eae0 Mon Sep 17 00:00:00 2001 From: Pan Bian Date: Wed, 28 Nov 2018 11:20:03 +0800 Subject: [PATCH 7/8] ubi: Do not drop UBI device reference before using The UBI device reference is dropped but then the device is used as a parameter of ubi_err. The bug is introduced in changing ubi_err's behavior. The old ubi_err does not require a UBI device as its first parameter, but the new one does. Fixes: 32608703310 ("UBI: Extend UBI layer debug/messaging capabilities") Signed-off-by: Pan Bian Reviewed-by: Boris Brezillon Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/kapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c index e9e9ecbcedcc..0b8f0c46268d 100644 --- a/drivers/mtd/ubi/kapi.c +++ b/drivers/mtd/ubi/kapi.c @@ -227,9 +227,9 @@ out_unlock: out_free: kfree(desc); out_put_ubi: - ubi_put_device(ubi); ubi_err(ubi, "cannot open device %d, volume %d, error %d", ubi_num, vol_id, err); + ubi_put_device(ubi); return ERR_PTR(err); } EXPORT_SYMBOL_GPL(ubi_open_volume); From e58725d51fa8da9133f3f1c54170aa2e43056b91 Mon Sep 17 00:00:00 2001 From: Richard Weinberger Date: Wed, 7 Nov 2018 23:04:43 +0100 Subject: [PATCH 8/8] ubifs: Handle re-linking of inodes correctly while recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UBIFS's recovery code strictly assumes that a deleted inode will never come back, therefore it removes all data which belongs to that inode as soon it faces an inode with link count 0 in the replay list. Before O_TMPFILE this assumption was perfectly fine. With O_TMPFILE it can lead to data loss upon a power-cut. Consider a journal with entries like: 0: inode X (nlink = 0) /* O_TMPFILE was created */ 1: data for inode X /* Someone writes to the temp file */ 2: inode X (nlink = 0) /* inode was changed, xattr, chmod, … */ 3: inode X (nlink = 1) /* inode was re-linked via linkat() */ Upon replay of entry #2 UBIFS will drop all data that belongs to inode X, this will lead to an empty file after mounting. As solution for this problem, scan the replay list for a re-link entry before dropping data. Fixes: 474b93704f32 ("ubifs: Implement O_TMPFILE") Cc: stable@vger.kernel.org Cc: Russell Senior Cc: Rafał Miłecki Reported-by: Russell Senior Reported-by: Rafał Miłecki Tested-by: Rafał Miłecki Signed-off-by: Richard Weinberger --- fs/ubifs/replay.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index a08c5b7030ea..0a0e65c07c6d 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -212,6 +212,38 @@ static int trun_remove_range(struct ubifs_info *c, struct replay_entry *r) return ubifs_tnc_remove_range(c, &min_key, &max_key); } +/** + * inode_still_linked - check whether inode in question will be re-linked. + * @c: UBIFS file-system description object + * @rino: replay entry to test + * + * O_TMPFILE files can be re-linked, this means link count goes from 0 to 1. + * This case needs special care, otherwise all references to the inode will + * be removed upon the first replay entry of an inode with link count 0 + * is found. + */ +static bool inode_still_linked(struct ubifs_info *c, struct replay_entry *rino) +{ + struct replay_entry *r; + + ubifs_assert(c, rino->deletion); + ubifs_assert(c, key_type(c, &rino->key) == UBIFS_INO_KEY); + + /* + * Find the most recent entry for the inode behind @rino and check + * whether it is a deletion. + */ + list_for_each_entry_reverse(r, &c->replay_list, list) { + ubifs_assert(c, r->sqnum >= rino->sqnum); + if (key_inum(c, &r->key) == key_inum(c, &rino->key)) + return r->deletion == 0; + + } + + ubifs_assert(c, 0); + return false; +} + /** * apply_replay_entry - apply a replay entry to the TNC. * @c: UBIFS file-system description object @@ -239,6 +271,11 @@ static int apply_replay_entry(struct ubifs_info *c, struct replay_entry *r) { ino_t inum = key_inum(c, &r->key); + if (inode_still_linked(c, r)) { + err = 0; + break; + } + err = ubifs_tnc_remove_ino(c, inum); break; }