From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751471AbdKLNr6 (ORCPT ); Sun, 12 Nov 2017 08:47:58 -0500 Received: from mail-lf0-f67.google.com ([209.85.215.67]:43170 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751146AbdKLNr4 (ORCPT ); Sun, 12 Nov 2017 08:47:56 -0500 X-Google-Smtp-Source: AGs4zMZsqCpSYuWaDacmjEoGzj1h3LUoukYA7m2QfWifaKTZOZFZH8QxCdnIyKbzwyKMwYkVM1ChZw== Date: Sun, 12 Nov 2017 14:47:59 +0100 From: Johan Hovold To: Marc Zyngier Cc: Johan Hovold , Thomas Gleixner , Jason Cooper , linux-kernel@vger.kernel.org, stable Subject: Re: [PATCH] irqchip/gic-v3: fix ppi-partitions lookup Message-ID: <20171112134759.GP11226@localhost> References: <20171111165125.27944-1-johan@kernel.org> <20171112123208.759b9a9d@why.wild-wind.fr.eu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171112123208.759b9a9d@why.wild-wind.fr.eu.org> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Nov 12, 2017 at 12:32:08PM +0000, Marc Zyngier wrote: > On Sat, 11 Nov 2017 17:51:25 +0100 > Johan Hovold wrote: > > Johan, > > > Fix child-node lookup during initialisation, which ended up searching > > the whole device tree depth-first starting at the parent rather than > > just matching on its children. > > > > To make things worse, the parent giq node was prematurely freed, while > > s/giq/gic/. > > Care to point out where that node would be prematurely freed? I don't > see your patch addressing that either... of_find_node_by_name() is used for tree-wide searches and, as documented, drops a reference to its first argument, which in this case is the parent gic node. > > the ppi-partitions node was leaked. > > > > Fixes: e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs") > > Cc: stable # 4.7 > > Do you have an example of this causing any trouble in the wild? As far > as I remember, the whole of_node refcounting isn't really enforced, so > while this is definitely a bug, it wouldn't cause any harm anywhere. Node refcounting is enabled with CONFIG_OF_DYNAMIC (e.g. when overlay support is enabled) and getting the refcounting wrong can lead to all sorts of issues like use-after-free and crashes. Using the wrong of-helper this way to lookup child nodes have been reproduced in several drivers, and I'm trying to fix them all up (and amend the kernel docs) to prevent this pattern from spreading further. In general you could end up matching and parsing an unrelated node with whatever implications that may have for a driver too. > Or am I missing something obvious? Feel free to drop the stable tag if you deem the implications for this particular driver to be benign. I can't test this one myself, but note that the node refcount is manipulated also after the unbalanced put (e.g. in gic_of_setup_kvm_info()). Thanks, Johan