On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> Since roles_init() adds some entries to the role hash table, we need to
> destroy also its keys/values on error, otherwise we get a memory leak in
> the error path.
>
> To avoid a forward declaration and maintain a sane layout, move all the
> destroy stuff above policydb_init. No changes are made to the moved code
> in this patch. Note that this triggers some pre-existing checkpatch.pl
> warnings - these will be fixed in follow-up patches.
>
> Reported-by: syzbot+fee3a14d4cdf92646287@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> 1 file changed, 489 insertions(+), 487 deletions(-)
Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
sure I would have suggested what I did, or rather I would have
suggested something slightly different.
When I ran my quick test when I was looking at your v1 patch, I only
moved perm_destroy() through ocontext_destroy(), leaving out
policydb_destroy(), and the diff was much more cleaner[*] (diffstat
below, includes the actual fix too). Could you try that and see if it
cleans up your patch?
security/selinux/ss/policydb.c | 378 +++++++++++++++++-----------------
1 file changed, 190 insertions(+), 188 deletions(-)
[*] In this case "cleaner" simply means that the moved lines were not
interleaved with existing code (just a big block of adds at the top,
the fix in the middle, and a big block of removals at the bottom).
--
paul moore
www.paul-moore.com
