Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] mkfs.ubifs: some error handling fixes
@ 2019-11-10 14:44 David Oberhollenzer
  2019-11-10 14:44 ` [PATCH 1/2] mkfs.ubifs: fscrypt: bail from encrypt_block if gen_essiv_salt fails David Oberhollenzer
  2019-11-10 14:44 ` [PATCH 2/2] mkfs.ubifs: abort add_directory if readdir fails David Oberhollenzer
  0 siblings, 2 replies; 3+ messages in thread
From: David Oberhollenzer @ 2019-11-10 14:44 UTC (permalink / raw)
  To: linux-mtd; +Cc: richard

Hi,

poking around the mkfs.ubifs source, I discovered two cases of what I
suspect to be error cases that got ignored instead of propperly handled.

The first is probably the more delicate one where the return value of
gen_essiv_salt is never checked in the fscrypt code.

The second one concerns pre-populating the ubifs image, where a readdir
loop is aborted if readdir fails, but the error status is never reported
back and the code simply marches on.

Please tell me if my fixes break something elsewhere or my assumptions
about the intended behaviour are flawed. Otherwise I will merge this some
time next week.

Thanks,

David


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH 1/2] mkfs.ubifs: fscrypt: bail from encrypt_block if gen_essiv_salt fails
  2019-11-10 14:44 [PATCH 0/2] mkfs.ubifs: some error handling fixes David Oberhollenzer
@ 2019-11-10 14:44 ` David Oberhollenzer
  2019-11-10 14:44 ` [PATCH 2/2] mkfs.ubifs: abort add_directory if readdir fails David Oberhollenzer
  1 sibling, 0 replies; 3+ messages in thread
From: David Oberhollenzer @ 2019-11-10 14:44 UTC (permalink / raw)
  To: linux-mtd; +Cc: richard, David Oberhollenzer

What originally cought my attention was that gen_essiv_salt has a
size_t return type and error paths that return -1 on failure.
Further investigation revealed that the error value is never checked
for. The encrypt_block function doesn't use the return value in any
way and simply continues onward.

Furthermore, the gen_essiv_salt function has an error case that emits
an error message but returns success state.

This patch modifes gen_essiv_salt to return an error status in all
error branches, changes the return type to ssize_t and adds a check
to encrypt_block if gen_essiv_salt fails.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 ubifs-utils/mkfs.ubifs/crypto.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ubifs-utils/mkfs.ubifs/crypto.c b/ubifs-utils/mkfs.ubifs/crypto.c
index cd68e37..d31bd2a 100644
--- a/ubifs-utils/mkfs.ubifs/crypto.c
+++ b/ubifs-utils/mkfs.ubifs/crypto.c
@@ -109,7 +109,7 @@ fail:
 	return -1;
 }
 
-static size_t gen_essiv_salt(const void *iv, size_t iv_len, const void *key, size_t key_len, void *salt)
+static ssize_t gen_essiv_salt(const void *iv, size_t iv_len, const void *key, size_t key_len, void *salt)
 {
 	size_t ret;
 	const EVP_CIPHER *cipher;
@@ -127,8 +127,10 @@ static size_t gen_essiv_salt(const void *iv, size_t iv_len, const void *key, siz
 	}
 
 	ret = do_encrypt(cipher, iv, iv_len, sha256, EVP_MD_size(EVP_sha256()), NULL, 0, salt);
-	if (ret != iv_len)
+	if (ret != iv_len) {
 		errmsg("Unable to compute ESSIV salt, return value %zi instead of %zi", ret, iv_len);
+		return -1;
+	}
 
 	free(sha256);
 
@@ -154,7 +156,8 @@ static ssize_t encrypt_block(const void *plaintext, size_t size,
 
 	if (cipher == EVP_aes_128_cbc()) {
 		tweak = alloca(ivsize);
-		gen_essiv_salt(&iv, FS_IV_SIZE, key, key_len, tweak);
+		if (gen_essiv_salt(&iv, FS_IV_SIZE, key, key_len, tweak) < 0)
+			return -1;
 	} else {
 		tweak = &iv;
 	}
-- 
2.21.0


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH 2/2] mkfs.ubifs: abort add_directory if readdir fails
  2019-11-10 14:44 [PATCH 0/2] mkfs.ubifs: some error handling fixes David Oberhollenzer
  2019-11-10 14:44 ` [PATCH 1/2] mkfs.ubifs: fscrypt: bail from encrypt_block if gen_essiv_salt fails David Oberhollenzer
@ 2019-11-10 14:44 ` David Oberhollenzer
  1 sibling, 0 replies; 3+ messages in thread
From: David Oberhollenzer @ 2019-11-10 14:44 UTC (permalink / raw)
  To: linux-mtd; +Cc: richard, David Oberhollenzer

The existing code sets 'err' to -1 and breaks the readdir loop, but
the error state is never read. This patch modifies the readdir loop
to actualy jump to the error handling branch if readdir fails.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 ubifs-utils/mkfs.ubifs/mkfs.ubifs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c b/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
index 5748aaa..72ae1b4 100644
--- a/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
+++ b/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
@@ -2043,8 +2043,7 @@ static int add_directory(const char *dir_name, ino_t dir_inum, struct stat *st,
 			if (errno == 0)
 				break;
 			sys_err_msg("error reading directory '%s'", dir_name);
-			err = -1;
-			break;
+			goto out_free;
 		}
 
 		if (strcmp(".", entry->d_name) == 0)
-- 
2.21.0


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-10 14:44 [PATCH 0/2] mkfs.ubifs: some error handling fixes David Oberhollenzer
2019-11-10 14:44 ` [PATCH 1/2] mkfs.ubifs: fscrypt: bail from encrypt_block if gen_essiv_salt fails David Oberhollenzer
2019-11-10 14:44 ` [PATCH 2/2] mkfs.ubifs: abort add_directory if readdir fails David Oberhollenzer

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git