linux-kernel - krealloc in kernel/params.c

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-next>] [day] [month] [year] [list]

Message-ID: <87mw8y8jq7.fsf@rasmusvillemoes.dk>
Date:	Tue, 14 Oct 2014 22:44:16 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	David Woodhouse <David.Woodhouse@...el.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:	linux-kernel@...r.kernel.org
Subject: krealloc in kernel/params.c

It is likely that I'm just missing something trivial, but I have
a hard time understanding 63662139e ("params: Fix potential
memory leak in add_sysfs_param()"). [I take it for granted that
krealloc() does not free its first argument when it fails, as
reading mm/slab_common.c seems to confirm.] Some of the relevant
lines from kernel/params.c:

    621         /* Enlarge. */
    622         new = krealloc(mk->mp,
    623                        sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1),
    624                        GFP_KERNEL);
    625         if (!new) {
    626                 kfree(attrs);
    627                 err = -ENOMEM;
    628                 goto fail;
    629         }
    630         /* Despite looking like the typical realloc() bug, this is safe.
    631          * We *want* the old 'attrs' to be freed either way, and we'll store
    632          * the new one in the success case. */
    633         attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL);
    634         if (!attrs) {
    635                 err = -ENOMEM;
    636                 goto fail_free_new;
    637         }
[...]
    663 fail_free_new:
    664         kfree(new);
    665 fail:
    666         mk->mp = NULL;
    667         return err;

First, if the krealloc call on line 622 fails, mk->mp seems to be
leaked (unless the caller happens to have a copy of that pointer
somewhere), since it is NULL'ed on the way out. 63662139e removed
a kfree(mk->mp) call. That doesn't seem right.

Second, the comment seems misleading, again because krealloc()
does not free its argument on failure. So if the krealloc() call
fails, the old attrs does seem to be leaked (attrs may be
mk->mp->grp.attrs, and again mk->mp is NULL'ed on the error
path (not that the caller could use mk->mp for anything - it is
invalidated by the successful first krealloc() call)).

Rasmus
--
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/