linux-kernel - Re: [PATCH] UBI: dereference after kfree in create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 5 May 2007 17:56:50 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	dedekind@...radead.org
Cc:	"Florin Malita" <fmalita@...il.com>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	linux-mtd@...ts.infradead.org,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] UBI: dereference after kfree in create_vtbl

On 5/5/07, Artem Bityutskiy <dedekind@...radead.org> wrote:
> [...]
> On Sat, 2007-05-05 at 09:25 +0530, Satyam Sharma wrote:
> > 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.
> 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.

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.

> > >though this likely requires a
> > major cleanup of this driver w.r.t. ubi_scan_leb lifetime semantics.
> What is wrong with the semantics, please be more specific.

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 :-)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/