Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754358AbbLKWDT (ORCPT ); Fri, 11 Dec 2015 17:03:19 -0500 Received: from down.free-electrons.com ([37.187.137.238]:42782 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753481AbbLKWDQ (ORCPT ); Fri, 11 Dec 2015 17:03:16 -0500 Date: Fri, 11 Dec 2015 23:03:05 +0100 From: Boris Brezillon To: Brian Norris Cc: David Woodhouse , linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Hartley Sweeten , Ryan Mallon , Shawn Guo , Sascha Hauer , Imre Kaloz , Krzysztof Halasa , Tony Lindgren , linux-omap@vger.kernel.org, Alexander Clouter , Thomas Petazzoni , Gregory CLEMENT , Jason Cooper , Sebastian Hesselbarth , Andrew Lunn , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Marek Vasut , Steven Miao , adi-buildroot-devel@lists.sourceforge.net, Mikael Starvik , Jesper Nilsson , linux-cris-kernel@axis.com, Josh Wu , Wan ZongShun , Ezequiel Garcia , Maxim Levitsky , Kukjin Kim , Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi@googlegroups.com, Stefan Agner , Greg Kroah-Hartman , devel@driverdev.osuosl.org, stable@vger.kernel.org, Dinh Nguyen Subject: Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove() Message-ID: <20151211230305.506e2071@bbrezillon> In-Reply-To: <20151211004008.GQ144338@google.com> References: <1449734442-18672-1-git-send-email-boris.brezillon@free-electrons.com> <1449734442-18672-2-git-send-email-boris.brezillon@free-electrons.com> <20151211004008.GQ144338@google.com> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3044 Lines: 80 Hi Brian, On Thu, 10 Dec 2015 16:40:08 -0800 Brian Norris wrote: > On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote: > > Unregister the NAND device from the NAND subsystem when removing a denali > > NAND controller, otherwise the MTD attached to the NAND device is still > > exposed by the MTD layer, and accesses to this device will likely crash > > the system. > > > > Signed-off-by: Boris Brezillon > > Cc: #3.8+ > > Does this follow these rules, from > Documentation/stable_kernel_rules.txt? > > - It must be obviously correct and tested. > > - It must fix a real bug that bothers people (not a, "This could be a > problem..." type thing). Sorry to bring the "stable or not stable (that is the question :-))" debate back, but after thinking a bit more about the implications of this missing nand_release() call, I think it is worth backporting the fix to all stable kernels. The reason is, it can potentially introduce a security hole, because if the mtd device is not unregister but the underlying mtd object is freed and the kernel reuses the same memory region for a different object, the MTD layer will possibly call one of the mtd->_method() function, and this field might point to another completely different function. You'll say that denali devices are probably never removed and this is the reason why people have never seen this problem before, which would be a good reason to not bother backporting the patch. But, given that the driver can be compiled as a module (the user can possibly load/unload it, which will in turn create/destroy the NAND/MTD device), and that the denali controller can be exposed through a PCI bus (which, AFAIK is hotpluggable), I really think this fix should be sent to stable. Best Regards, Boris > > > Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer") > > --- > > drivers/mtd/nand/denali.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > > index 67eb2be..8feece3 100644 > > --- a/drivers/mtd/nand/denali.c > > +++ b/drivers/mtd/nand/denali.c > > @@ -1622,6 +1622,7 @@ EXPORT_SYMBOL(denali_init); > > /* driver exit point */ > > void denali_remove(struct denali_nand_info *denali) > > { > > + nand_release(&denali->mtd); > > denali_irq_cleanup(denali->irq, denali); > > dma_unmap_single(denali->dev, denali->buf.dma_buf, > > denali->mtd.writesize + denali->mtd.oobsize, > > It feels a bit odd to allow usage of MTD fields after it has been > unregistered. Maybe precompute this before the nand_release()? > > Brian -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/