| Message ID | 1449734442-18672-2-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <boris.brezillon@free-electrons.com> > Cc: <stable@vger.kernel.org> #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). > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, On Thu, 10 Dec 2015 16:40:08 -0800 Brian Norris <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > Cc: <stable@vger.kernel.org> #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). As you wish, I'll remove those Cc and Fixes tags, or just drop the patch if you think it's useless... I just noticed the bug while reworking this series, and thought it would be useful to fix it, but I honestly don't care if it's applied or not (I don't use this platform). > > > 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()? nand_realease() is not releasing the mtd instance or re-initialazing its field, so it should be safe, but I agree that pre-computing the DMA buffer size is more future-proof. I'll fix that, send a v5 and let you decide whether it's needed or not. Best Regards, Boris
On Fri, Dec 11, 2015 at 02:53:20PM +0100, Boris Brezillon wrote: > Hi Brian, > > On Thu, 10 Dec 2015 16:40:08 -0800 > Brian Norris <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > > Cc: <stable@vger.kernel.org> #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). > > As you wish, I'll remove those Cc and Fixes tags, or just drop the > patch if you think it's useless... The fixes tag is a separate thing from CCing stable. It's useful on by itself. I always put the person who wrote the original patch in the To: header so they can review and comment if I have made a mistake. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dan, On Fri, 11 Dec 2015 17:39:47 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Fri, Dec 11, 2015 at 02:53:20PM +0100, Boris Brezillon wrote: > > Hi Brian, > > > > On Thu, 10 Dec 2015 16:40:08 -0800 > > Brian Norris <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > > > Cc: <stable@vger.kernel.org> #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). > > > > As you wish, I'll remove those Cc and Fixes tags, or just drop the > > patch if you think it's useless... > > The fixes tag is a separate thing from CCing stable. It's useful on by > itself. I always put the person who wrote the original patch in the To: > header so they can review and comment if I have made a mistake. Noted. I added back the Fixes tag and added Dinh Nguyen (the commit author) in the loop. Thanks, Boris
Hi Brian, On Thu, 10 Dec 2015 16:40:08 -0800 Brian Norris <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > Cc: <stable@vger.kernel.org> #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
Hi Boris, On Fri, Dec 11, 2015 at 11:03:05PM +0100, Boris Brezillon wrote: > On Thu, 10 Dec 2015 16:40:08 -0800 > Brian Norris <computersforpeace@gmail.com> 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 <boris.brezillon@free-electrons.com> > > > Cc: <stable@vger.kernel.org> #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. That's all well and good, but still nobody has told me they've tested this. I've pushed your v5 (+ comments, + ack) to l2-mtd.git. If it gets testing and this request is made again at that point, we can easily send it to stable after it hits Linus' tree. See option 2 in Documentation/stable_kernel_rules.txt. You can even send the email yourself, just CC me and anyone else relevant. I'll ack it if it's been tested. Regards, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 <boris.brezillon@free-electrons.com> Cc: <stable@vger.kernel.org> #3.8+ Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer") --- drivers/mtd/nand/denali.c | 1 + 1 file changed, 1 insertion(+)