linux-ext4 - Re: [PATCH] e2fsprogs: make ext2fs_free() to set the pointer to NULL

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:	Wed, 19 Feb 2014 12:57:14 -0500
From:	Theodore Ts'o <tytso@....edu>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] e2fsprogs: make ext2fs_free() to set the pointer to NULL

On Wed, Feb 19, 2014 at 12:48:16PM +0100, Lukas Czerner wrote:
> Currently someone uses ext2fs_free() to free the ext2_filsys structure,
> he is also responsible to set the pointer to that structure to NULL,
> because ext2fs_free() is not able to do that.
> 
> This however is in contrast with ext2fs_free_mem() which requires as an
> argument pointer to the pointer and it will in fact set the pointer to
> NULL for us. This is probably the reason why majority of places where we
> use ext2fs_free() does not set the pointer to NULL afterwards.
> 
> Fix this by changing function ext2fs_free() so that it'll require
> pointer to the ext2_filsys as an argument and will be able to set the
> pointer to NULL for us.
> 
> I do not currently have any way to trigger the issue in recent
> e2fsprogs. Part of the reason is that there are some fixes which just
> obfuscates the real problem (for example
> 7ff040f30f0ff3bf5e2c832da3cb577e00a52d60) and the other part might be
> just coincidence.
> 
> I was however able to reproduce this with e2fsprogs 1.42.9 using the
> image in bug https://bugzilla.redhat.com/show_bug.cgi?id=997982. With
> this patch it fixes it. However I am not sure why it got fixed in the
> recent e2fsprogs since git bisect lands into the merge commit - however
> I think it's just a coincidence rather than targeted fix.
> 
> Signed-off-by: Lukas Czerner <lczerner@...hat.com>

This results in an ABI change, so if there are any userspace programs
which are linked against the libext2fs shared library, it would break.

So we have two choices:

1) Audit all of the callers of ext2fs_freefs to see if it's necessary
to clear the pointer.  If the pointer is about to be overwritten, or
it's on a stack-allocated variable that will disappear as soon as you
return, it's not a problem.  Given that your patch had to change every
single ext2fs_free() call stack, it's relatively easy to make sure we
don't miss any.  :-)

2)  Define a new ext2fs_freefs2() which takes the changed interface.

I'm not entirely convinced that (2) is worth it, but certainly (1)
would be a good thing to do.

It may be that the problem you couldn't replicate in the latest
version of e2fsprogs was one that was fixed either when I did my
periodic valgrind test runs, or as a result of reviewing all of the
coverity warnings.

Cheers,

							- Ted

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html