Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753200AbdLEP1U (ORCPT ); Tue, 5 Dec 2017 10:27:20 -0500 Received: from albert.telenet-ops.be ([195.130.137.90]:49490 "EHLO albert.telenet-ops.be" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753087AbdLEP1I (ORCPT ); Tue, 5 Dec 2017 10:27:08 -0500 From: Geert Uytterhoeven To: Pantelis Antoniou , Rob Herring , Frank Rowand Cc: Colin King , Dan Carpenter , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Geert Uytterhoeven Subject: [PATCH v3 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path Date: Tue, 5 Dec 2017 16:27:02 +0100 Message-Id: <1512487623-30450-2-git-send-email-geert+renesas@glider.be> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1512487623-30450-1-git-send-email-geert+renesas@glider.be> References: <1512487623-30450-1-git-send-email-geert+renesas@glider.be> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2748 Lines: 87 If of_resolve_phandles() fails, free_overlay_changeset() is called in the error path. However, that function returns early if the list hasn't been initialized yet, before freeing the object. Explicitly calling kfree() instead would solve that issue. However, that complicates matter, by having to consider which of two different methods to use to dispose of the same object. Hence make free_overlay_changeset() consider initialization state of the different parts of the object, making it always safe to call (once!) to dispose of a (partially) initialized overlay_changeset: - Only destroy the changeset if the list was initialized, - Make init_overlay_changeset() store the ID in ovcs->id on success, to avoid calling idr_remove() with an error value or an already released ID. Reported-by: Colin King Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays") Signed-off-by: Geert Uytterhoeven --- v3: - Store into ovcs->id on success only, drop id > 0 check before release, v2: - New. --- drivers/of/overlay.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index c150abb9049d776d..bdb9695ed2d889a7 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -522,7 +522,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, struct device_node *node, *overlay_node; struct fragment *fragment; struct fragment *fragments; - int cnt, ret; + int cnt, id, ret; /* * Warn for some issues. Can not return -EINVAL for these until @@ -543,9 +543,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, of_changeset_init(&ovcs->cset); - ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL); - if (ovcs->id <= 0) - return ovcs->id; + id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL); + if (id <= 0) + return id; cnt = 0; @@ -611,6 +611,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, goto err_free_fragments; } + ovcs->id = id; ovcs->count = cnt; ovcs->fragments = fragments; @@ -619,7 +620,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, err_free_fragments: kfree(fragments); err_free_idr: - idr_remove(&ovcs_idr, ovcs->id); + idr_remove(&ovcs_idr, id); pr_err("%s() failed, ret = %d\n", __func__, ret); @@ -630,9 +631,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) { int i; - if (!ovcs->cset.entries.next) - return; - of_changeset_destroy(&ovcs->cset); + if (ovcs->cset.entries.next) + of_changeset_destroy(&ovcs->cset); if (ovcs->id) idr_remove(&ovcs_idr, ovcs->id); -- 2.7.4