UBI: dereference after kfree in create_vtbl

Coverity (CID 1614) spotted new_seb being dereferenced after kfree() in create_vtbl's write_error path.

---

vtbl.c | 11 +++++------

1 file changed, 5 insertions(+), 6 deletions(-)

index b6fd6bb..91e3619 100644
@@ -317,14 +317,13 @@ retry:

	return err;

write_error:
- kfree(new_seb);
- /* May be this physical eraseblock went bad, try to pick another one */
- if (++tries <= 5) {
+ /* Maybe this physical eraseblock went bad, try to pick another one */
+ if (++tries <= 5)

		err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
					   &si->corr);

- if (!err)
- goto retry;
- }
+ kfree(new_seb);
+ if (!err)
+ goto retry;
out_free:

	ubi_free_vid_hdr(ubi, vid_hdr);
	return err;

Applied with minor trailing white-space cleanup, thanks.

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Hi Florin, Artem,

1 file changed, 3 insertions(+), 5 deletions(-)

diff -ruNp a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
@@ -317,14 +317,12 @@ retry:

	return err;

-	kfree(new_seb);
	/* May be this physical eraseblock went bad, try to pick another one */

		if (!err)
			goto retry;
-	}
out_free:
	ubi_free_vid_hdr(ubi, vid_hdr);
	return err;

Hi Satyam,

Not being familiar with the code, I was specifically trying to preserve the old semantics and only address the use-after-free issue. So if there was another bug... well, I guess I succeeded at preserving it ;)

[snip]

index b6fd6bb..2ad2d59 100644
@@ -317,14 +317,10 @@ retry:

	return err;

	ubi_free_vid_hdr(ubi, vid_hdr);
	return err;

You're absolutely right. We must preserve and return the original (write error) return code and not the spurious ENOMEM / 0 that would come from ubi_scan_add_to_list. I had noticed this too, but this time _I_ preserved the (flawed) old semantics which leads to this problem. Wow ... so you actually found 3 bugs here in ~3 lines of code :-)

I tried going deeper into this, but the lifetime semantics of a struct ubi_scan_leb in this driver are quite ... weird indeed.

Again, I saw that too, but would still prefer using the higher level function ubi_scan_add_to_list() to add to the corrupted list, but with a different identifier for the return value to avoid overwriting err. list_add_tail seems best left as an implementation detail below ubi_scan_add_to_list(), IMO. So if it fails in the error path, we'd have to return with the original (write error) return value and the ENOMEM sort of goes ... unreturned. Alas!

Artem would have to step in here to verify if there really is a good reason why we kmalloc a fresh ubi_scan_leb every time we want to add one to a list. If possible, the best solution would be to change ubi_scan_add_to_list() to take in a valid struct ubi_scan_leb and just add that to the specified list (using list_add_tail or whatever) -- and leave allocation up to callers, though this likely requires a major cleanup of this driver w.r.t. ubi_scan_leb lifetime semantics. ubi_scan_add_used() was such a horror!

If adding an existing ubi_scan_leb is fine, but we can't really change ubi_scan_add_to_list right now, then I'd much rather see a __ubi_scan_add_to_list() that does the list_add_tail (and those associated debug printk messages: btw are those printk's the only reason why we're carrying that ubi_scan_info into ubi_scan_add_to_list?). So ubi_scan_add_to_list() just wraps a kmalloc over it -- that way, callers who want a new eraseblock alloced before adding it to a list can use ubi_scan_add_to_list() and those that already hold a valid one (like us) could use __ubi_scan_add_to_list() directly.

I see Andrew merged the previous fix, so this is on top of that, though I'm not sure if this half-solution should actually go in -- hopefully that verbose comment / changelog will irritate Artem enough to fix all this for good :-)

---

drivers/mtd/ubi/vtbl.c:create_vtbl() uses ubi_scan_add_to_list() in the write error path. That can itself return with an ENOMEM error, in which case we give up but return from create_vtbl() with the original write error.

A robust solution would be to fix ubi_scan_add_to_list() to just take in a valid eraseblock and simply add it to the specified list using list_add_tail, and thus never fail. This would likely require a major cleanup of this driver.

Alternatively, we could introduce and use a void __ubi_scan_add_to_list() here that does this and make ubi_scan_add_to_list() simply wrap a kmalloc over it, to avoid changing existing users.

---

drivers/mtd/ubi/vtbl.c | 17 ++++++++++++-----

1 file changed, 12 insertions(+), 5 deletions(-)

---

diff -ruNp a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
@@ -260,7 +260,7 @@ bad:

static int create_vtbl(const struct ubi_device *ubi, struct ubi_scan_info *si,
int copy, void *vtbl)
{
- int err, tries = 0;

+	int add_err, err, tries = 0;
	static struct ubi_vid_hdr *vid_hdr;
	struct ubi_scan_volume *sv;
	struct ubi_scan_leb *new_seb, *old_seb = NULL;
@@ -317,12 +317,19 @@ retry:
	return err;

write_error:
- /* May be this physical eraseblock went bad, try to pick another one */
- err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, &si->corr);
+ /*
+ * May be this physical eraseblock went bad, so add it to the corrupted
+ * list. Note that ubi_scan_add_to_list() allocates a new eraseblock
+ * and could fail with ENOMEM. In that case we give up and return with
+ * the original write error -- the ENOMEM is effectively lost.
+ */

+	add_err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, &si->corr);
	kfree(new_seb);
+	if (add_err)
+		goto out_free;
+	/* Try to pick another one */
	if (++tries <= 5)

- if (!err)
- goto retry;
+ goto retry;
out_free:

	ubi_free_vid_hdr(ubi, vid_hdr);
	return err;

Hi,

thanks for finding bugs in this patch. Although this path will likely never happen, this is good to have it bug-free.

Particularly in vtbl.c there is no good reason. Leftover of itsy-bitsy units. I'll make ubi_scan_add_to_list static, as well as ubi_scan_add_used(). And I'll rename them to something shorter. They are only useful in scan.c.

And it is fine to use list_add_tail() directly in vtbl.c. Will be fixed.

In scan.c it is useful because _all_ callers have to allocate it. vtbl.c is the only place which does not need it. I'll fix this.

What is wrong with the semantics, please be more specific.

I'll fix this shortly.

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Ah, good to know that, but please keep list_add_tail (or whatever is the implementation abstraction of the various ubi_scan_info lists) local to scan.c -- you could expose a version of ubi_scan_add_to_list that does not do kmalloc through scan.h and use that in vtbl.c. That way you won't lose those debug printk's when adding an eraseblock to a list, for example, and it's always much cleaner not exposing an object's implementation innards to others.

It's wrong to be allocating and freeing any object in more than one place . Physical eraseblocks are allocated in ubi_scan_add_to_list (which shouldn't be doing that) and ubi_scan_add_used (which is a maze) and freed pretty much all over the place (because we allocate new seb's for ourselves to add to the lists, we need to go about kfree'ing all of them when destroying a ubi_scan_destroy_si too, for example -- perhaps this driver needs to be told about krefs). So it makes life easier if you know there's only one place when/where an object is born, if you know that it'll remain alive as long as you need it and have a reference on it, and if you destroy it a known single time/location too. I wish I could be more specific than this, but I've only spent a few hours with ubi :-)

It's error path and that print is not really needed. We'll see other complaints in that case. And this is _the only_ place outside scan.c, so it makes sense to use list_add_tail(). We do not really need to hide this behind some other call (ubi_scan_add_to_list())

Yes, per-PEB scanning information is allocated in ubi_scan_add_to_list() and ubi_scan_add_to_used(). I do not see why it shouldn't be doing that.

It actually is rather complex because it does a rather complex thing. But any patch/idea to make it simpler is welcome.

It is only freed in ubi_scan_destroy_si(). Yes, there is one exception in create_vtbl, but this is because I did not want to introduce any special version of ubi_scan_add_used().

It does not hurt at all that we do one extra allocation, because it is called _only_ 2 times (once for each volume table copy).

Sorry. not sure what you mean. They are allocated in 2 places, and freed in one, with one exception in vtbl_create() which does not matter much.

May be it is, but I have 2 places and do not see any problem.

I have 1 destroy location. And one exception where I allocate it temporarily and destroy in the same function. And it is done only 2 times and only if one attaches un-formatted flash.

Thanks for your analysis, it would be helpful if more people did this.

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Well, you're developing / maintaining this right now, so it's your call. Though I bet most people would find keeping that list_add_tail local to scan.c more tasteful.

I wish you'd commented it better than "This function returns zero in case of success and a negative error code in case of failure." in that case :-)

Again, you're developing and maintaining this right now, so it's your call. Though it would be easier on you if you remove these exceptions that could be quite easily removed, actually.

Thanks,
Satyam

I do not think so. If you are interested, try to find "UBI take 2" patches in lkml. Look how it looked liked. It consisted of many independent units and units could access other units _only_ via interfaces. I would do what you say there.

Read Teo's comments - I actually now agree with them. And after I had changed UBI i got rid of several thousands lines of code, and the code became simpler.

So, my argument is:
1. It makes no sense to introduce one more non-static function to _just_ encapsulate list_add_tail and _just_ for one caller.
2. It is _C_, it is _kernel_, and it is OK sometimes _not_ to follow computer since rules.

Agreed, I'll add more comments, thanks.

I do not see any nice way to do this. If you suggest one, I will do.

Best regards,
Artem Bityutskiy (Битюцкий Артём)

But what you're doing now is *worse*, don't you see it? You _think_ that you are not exporting any add_to_list function from scan.c because you removed the wrapper from scan.h, but then you open-code it anyway in create_vtbl (yes, that list_add _is_ the *only* meaningful code in add_to_list; it just so _happens_ that most of its callers till now also wanted to allocate a ubi_scan_leb too at that time so you pushed that too into add_to_list rather than do it in the callers -- via a separate alloc_leb, for example). Now that means there is an *undocumented* use of the "add to list" _functionality_ outside of scan.c anyway and which wouldn't even come up if someone tries to analyse / overhaul the code some day in the future. I know it's just _one_ exception, but still, I'm a heckler for style.

Well, introducing one more non-static function is *not* what I originally had in mind (it was proposed only as a temporary measure till all users of ubi_scan_add_to_list could be updated to use the other version, explained below).

I actually planned to _replace_ ubi_scan_add_to_list with another version that does *not* allocate memory (the __ubi_scan_add_to_list I proposed above), and leave allocation up to its *callers* who therefore pass a valid ubi_scan_leb to it. But that is where I ran into ubi_scan_add_used().

Ah, well, forget it. It's all a matter of taste and maintainability. I guess in the end it needs to be fine with you.

I've put the fix here:
http://git.infradead.org/?p=users/dedekind/ubi-2.6.git;a=commit;h=5125237efb6a3309fbf5b9a7a21aaf716787f2a2

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Ummm ...

1. "if (err == -EIO)" applies to adding new_seb to the corrupted list, and not to retrying. We wouldn't want _not_ to retry if there's some other error, or would we?
2. "if (++tries <= 5)" applies to "goto retry" and not to adding new_seb to the corrupted list. If we hit write failure for the 5th time and err == -EIO, we should still be adding it to corrupted list, but not retry, of course. Otherwise we would add the first 4 write failure (with -EIO) eraseblocks to si->corr, but the 5th _similar_ case is ... just freed?

So:

write_error:

	/*
	 * Probably this physical eraseblock went bad, add it to the
	 * corrupted list. Else, just release it.
	 */
	if (err == -EIO)
		/* list_add_tail(&new_seb->u.list, &si->corr); */
		__ubi_scan_add_to_list(si, new_seb, &si->corr);
	else
		kfree(new_seb);
	/* Try to pick another one */
	if (++tries <= 5)
		goto retry;
out_free:
	ubi_free_vid_hdr(ubi, vid_hdr);
	return err;

And in case you want to avoid the list_add_tail there, the following (I'm fine with whichever way, of course):

diff -ruNp a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
@@ -55,29 +55,37 @@ static int paranoid_check_si(const struc

static struct ubi_ec_hdr *ech;
static struct ubi_vid_hdr *vidh;

-int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,
- struct list_head *list)
+void __ubi_scan_add_to_list(struct ubi_scan_info *si,
+ struct ubi_scan_leb *seb,
+ struct list_head *list)
{
- struct ubi_scan_leb *seb;
-

	if (list == &si->free)
-		dbg_bld("add to free: PEB %d, EC %d", pnum, ec);
+		dbg_bld("add to free: PEB %d, EC %d", seb->pnum, seb->ec);
	else if (list == &si->erase)
-		dbg_bld("add to erase: PEB %d, EC %d", pnum, ec);
+		dbg_bld("add to erase: PEB %d, EC %d", seb->pnum, seb->ec);
	else if (list == &si->corr)
-		dbg_bld("add to corrupted: PEB %d, EC %d", pnum, ec);
+		dbg_bld("add to corrupted: PEB %d, EC %d", seb->pnum, seb->ec);
	else if (list == &si->alien)
-		dbg_bld("add to alien: PEB %d, EC %d", pnum, ec);
+		dbg_bld("add to alien: PEB %d, EC %d", seb->pnum, seb->ec);
	else
		BUG();

+ list_add_tail(&seb->u.list, list);
+}
+
+int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,
+ struct list_head *list)
+{
+ struct ubi_scan_leb *seb;
+

	seb = kmalloc(sizeof(struct ubi_scan_leb), GFP_KERNEL);
	if (unlikely(!seb))
		return -ENOMEM;

	seb->pnum = pnum;
	seb->ec = ec;
-	list_add_tail(&seb->u.list, list);
+	__ubi_scan_add_to_list(si, seb, list);
+
	return 0;
}

diff -ruNp a/drivers/mtd/ubi/scan.h b/drivers/mtd/ubi/scan.h
@@ -147,6 +147,9 @@ static inline void ubi_scan_move_to_list

		list_add_tail(&seb->u.list, list);
}

+void __ubi_scan_add_to_list(struct ubi_scan_info *si,
+ struct ubi_scan_leb *seb,
+ struct list_head *list);
int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,

			 struct list_head *list);

int ubi_scan_add_used(const struct ubi_device *ubi, struct ubi_scan_info *si,

In case of other error - no, we do not want to retry. Only in case of -EIO because we just might have hit a new badblock, which is unlikely, but possible.

If it is anything else then -EIO, then we just return an error and _refuse_ to attach this MTD device. In this case it does not matter where we add new_seb. We just drop it. We free all allocated data structures.

If we hit -EIO more then five times, there is probably something _really bad_ with this MTD device and we _refuse_ attaching it. We return error, and every data structure is freed. It does not matter if we add new_seb anywhere or not. It is anyway just freed.

Best regards,
Artem Bityutskiy (Битюцкий Артём)