linux-kernel - RE: [PATCH] mtd: use refcount to prevent corruption

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

Date:   Thu, 28 Jan 2021 17:57:50 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
CC:     Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mtd: use refcount to prevent corruption

> Hi Tomas,
> 
> "Winkler, Tomas" <tomas.winkler@...el.com> wrote on Thu, 28 Jan 2021
> 08:53:43 +0000:
> 
> > > Tomas,
> > >
> > > ----- Ursprüngliche Mail -----
> > > >> >> Can you please explain a little more what devices are involved?
> > > >> >> Does it implement _get_device() and _put_device()?
> > > >> > No this is not connected to those handlers of the underlying
> > > >> > device and those won't help.
> > > >> > I have a spi device provided by MFD framework so it can go away
> anytime.
> > > >>
> > > >> Can it go away physically or just in software?
> > > > Software, but since this is mfd it's basically hotplug. The kernel
> > > > is crashing when I simulate hardware failure.
> > > >>
> > > >> Usually the pattern is that you make sure in the device driver
> > > >> that nobody can orphan the MTD while it is in use.
> > > >> e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs
> > > >> a reference on the underlying UBI volume to make sure it cannot
> > > >> go away while the MTD (on top of UBI) is in use.
> > > >
> > > > I can try that if it helps, because we are simulating possible
> > > > lower level crash.
> > > > In an case I believe that the proper refcouting is much more
> > > > robust solution, than the current one.
> > > > I'd appreciate if someone can review the actual implementation.
> > >
> > > This happens right now, I try to understand why exactly the current
> > > way is not good in enough. :-)
> > >
> > > Your approach makes sure that the MTD itself does not go away while
> > > it has users but how does this help in the case where the underlying
> > > MFD just vanishes?
> > > The MTD can be in use and the MFD can go away while e.g. mtd_read()
> > > or such takes place.
> >
> > Read will fail, but kernel won't crash on access to memory that was freed.
> 
> As Richard was saying, we are really open to enhance MTD refcounting.
> 
> However, the issue you are facing is, IMHO, not related to MTD but to MFD.
> There should be a way to avoid MFD to vanish by taking a reference of it
> through mtd->_get_device(). I don't think addressing the case where MFD
> vanishes while MTD (as a user) is still active is the right approach.

I think it won't work because MFD sub-driver remove() is called   and it must succeed because the main device  is not accessible unlike glueubi which just returns -EBUSY.
so we postpone the mtd unregister to  mtd_info->_put_device()  but it that state we have nothing to hold
on as the device is gone in remove()
User will fail anyway, as the underlying device is not functional in that state.
Anyway I've tried your suggestion, the kernel is crashing, hope I haven't done some silly bug.

Thanks
Tomas