linux-kernel - Re: [REVIEW] NVM Express driver

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]

Message-ID: <20110304221034.GD25574@kroah.com>
Date:	Fri, 4 Mar 2011 14:10:34 -0800
From:	Greg KH <greg@...ah.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Matthew Wilcox <willy@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [REVIEW] NVM Express driver

On Fri, Mar 04, 2011 at 09:59:15PM +0000, Alan Cox wrote:
> > And non-automated loading of firmware as well.  Dell uses this for
> > updating their BIOSes just fine, with a userspace tool that initiates
> > the loading of the firmware.
> 
> Try using the Dell tool with namespaces.

Heck, try using _any_ tool that talks to sysfs with namespaces, don't
try to claim that this driver using its own custom firmware loader is a
solution to the namespace/sysfs issue please.  That's very disingenuous.

> > How does Dell do it?
> 
> How do most other apps do it.

They write to the sysfs file with the firmware when they feel like it.

> > So, what could be changed in the current firmware interface to fix this
> > problem in a manner which would solve these perceived issues?
> 
> I'm not sure it can. The basis of the interface is driver requests
> firmware. That is done by using a pathname which in a namespaced
> environment isn't global and has races

Of course, but those races don't show up in real systems, right?

> (Several things break quite spectacularly if you request_firmware while
> updating a package, but of course nobody considered such details even for
> automatic stuff in many cases. Really there is some locking needed).

I've never heard of this race before as you usually do firmware upload
either at boot time, or when a user specifically asks for it.  Neither
of those times is when packages are usually getting updated.

> For manual updating of a block of firmware the interface most stuff wants
> is
> 
> 	copy_from_user()
> 
> or if you wanted to wrap it up nicely
> 
> 	x = vmalloc_from_user(void __user *ptr, ssize_t len);
> 
> The app knows which firmware it is talking about and can inspect and
> compare it while holding an open file handle on the deivce. That protects
> against hotplug races and getting the wrong device second access, and
> ensures that the firmware, device and userspace are all talking about
> exactly the same thing.

But you do get this type of buffer from the firware interface to your
driver today, right?

> It would nice to say "its an obscure corner case that will just error",
> but far too much hardware still gets semi permanently (or permanently)
> converted into junk if you goof a permanent flash firmware update.

One would hope that the hardware was a bit more resiliant than that, but
I know how hardware is designed :(

Still, I don't want this to all of a sudden be "open season" for every
individual driver deciding to want to create an ioctl call for firmware
updating just because the authors don't like the existing in-kernel
interface.  Please fix up the in-kernel one instead to meet your needs.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/